Horton

Thread

Commits

  1. Handle NULL fields in WRITE_INDEX_ARRAY

  1. Fix NULL pointer reference in _outPathTarget()

    Richard Guo <guofenglinux@gmail.com> — 2022-04-18T07:35:53Z

    The array sortgrouprefs[] inside PathTarget might be NULL if we have not
identified sort/group columns in this tlist. In that case we would have
a NULL pointer reference in _outPathTarget() when trying to print
sortgrouprefs[] with WRITE_INDEX_ARRAY as we are using the length of
PathTarget->exprs as its array length.

Attached is a fix that can address this problem.

Thanks
Richard

  2. Re: Fix NULL pointer reference in _outPathTarget()

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-18T18:53:41Z

    Richard Guo <guofenglinux@gmail.com> writes:
> The array sortgrouprefs[] inside PathTarget might be NULL if we have not
> identified sort/group columns in this tlist. In that case we would have
> a NULL pointer reference in _outPathTarget() when trying to print
> sortgrouprefs[] with WRITE_INDEX_ARRAY as we are using the length of
> PathTarget->exprs as its array length.

I wondered why we'd not noticed this long since, and the answer is that
it got broken relatively recently by bdeb2c4ec, which removed the former
conditionality of the code:

@@ -2510,14 +2517,7 @@ _outPathTarget(StringInfo str, const PathTarget *node)
     WRITE_NODE_TYPE("PATHTARGET");
 
     WRITE_NODE_FIELD(exprs);
-    if (node->sortgrouprefs)
-    {
-        int            i;
-
-        appendStringInfoString(str, " :sortgrouprefs");
-        for (i = 0; i < list_length(node->exprs); i++)
-            appendStringInfo(str, " %u", node->sortgrouprefs[i]);
-    }
+    WRITE_INDEX_ARRAY(sortgrouprefs, list_length(node->exprs));
     WRITE_FLOAT_FIELD(cost.startup, "%.2f");
     WRITE_FLOAT_FIELD(cost.per_tuple, "%.2f");
     WRITE_INT_FIELD(width);

A semantics-preserving conversion would have looked something like

    if (node->sortgrouprefs)
        WRITE_INDEX_ARRAY(sortgrouprefs, list_length(node->exprs));

I suppose that Peter was trying to remove special cases from the
outfuncs.c code, but do we want to put this one back?  Richard's
proposal would not accurately reflect the contents of the data
structure, so I'm not too thrilled with it.

			regards, tom lane

  3. Re: Fix NULL pointer reference in _outPathTarget()

    Richard Guo <guofenglinux@gmail.com> — 2022-04-19T02:51:49Z

    On Tue, Apr 19, 2022 at 2:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

>
> A semantics-preserving conversion would have looked something like
>
>     if (node->sortgrouprefs)
>         WRITE_INDEX_ARRAY(sortgrouprefs, list_length(node->exprs));
>
> I suppose that Peter was trying to remove special cases from the
> outfuncs.c code, but do we want to put this one back?  Richard's
> proposal would not accurately reflect the contents of the data
> structure, so I'm not too thrilled with it.
>

The commit message in bdeb2c4ec mentions that:

"
This also changes the behavior slightly: Before, the field name was
skipped if the length was zero.  Now it prints the field name even in
that case.  This is more consistent with how other array fields are
handled.
"

So I suppose we are trying to print the field name even if the length is
zero. Should we keep this behavior in the fix?

Thanks
Richard

  4. Re: Fix NULL pointer reference in _outPathTarget()

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-04-19T09:53:33Z

    On 2022-Apr-18, Tom Lane wrote:

> I suppose that Peter was trying to remove special cases from the
> outfuncs.c code, but do we want to put this one back?  Richard's
> proposal would not accurately reflect the contents of the data
> structure, so I'm not too thrilled with it.

Yeah -- looking at the script to generate node support functions[1], it
might be better go back to the original formulation (i.e., your proposed
patch), and then use a "path_hack4" for this struct member, which looks
similar to other hacks already there for other cases that require
bespoke handling.

[1] https://postgr.es/m/bee9fdb0-cd10-5fdb-3027-c4b5a240bc74@enterprisedb.com

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  5. Re: Fix NULL pointer reference in _outPathTarget()

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-04-20T16:02:47Z

    On 18.04.22 09:35, Richard Guo wrote:
> The array sortgrouprefs[] inside PathTarget might be NULL if we have not
> identified sort/group columns in this tlist. In that case we would have
> a NULL pointer reference in _outPathTarget() when trying to print
> sortgrouprefs[] with WRITE_INDEX_ARRAY as we are using the length of
> PathTarget->exprs as its array length.

Do you have a test case that triggers this issue?

  6. Re: Fix NULL pointer reference in _outPathTarget()

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-04-20T16:04:00Z

    On 18.04.22 20:53, Tom Lane wrote:
> A semantics-preserving conversion would have looked something like
> 
>      if (node->sortgrouprefs)
>          WRITE_INDEX_ARRAY(sortgrouprefs, list_length(node->exprs));
> 
> I suppose that Peter was trying to remove special cases from the
> outfuncs.c code, but do we want to put this one back?  Richard's
> proposal would not accurately reflect the contents of the data
> structure, so I'm not too thrilled with it.

I think we could put the if (node->fldname) inside the WRITE_INDEX_ARRAY 
macro.

  7. Re: Fix NULL pointer reference in _outPathTarget()

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-20T16:53:50Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 18.04.22 20:53, Tom Lane wrote:
>> A semantics-preserving conversion would have looked something like
>> 	if (node->sortgrouprefs)
>> 		WRITE_INDEX_ARRAY(sortgrouprefs, list_length(node->exprs));

> I think we could put the if (node->fldname) inside the WRITE_INDEX_ARRAY 
> macro.

Yeah, that's another way to do it.  I think though that the unresolved
question is whether or not we want the field name to appear in the output
when the field is null.  I believe that I intentionally made it not appear
originally, so that that case could readily be distinguished.  You could
argue that that would complicate life greatly for a _readPathTarget()
function, which is true, but I don't foresee that we'll need one.

			regards, tom lane

  8. Re: Fix NULL pointer reference in _outPathTarget()

    Richard Guo <guofenglinux@gmail.com> — 2022-04-21T04:25:11Z

    On Thu, Apr 21, 2022 at 12:02 AM Peter Eisentraut <
peter.eisentraut@enterprisedb.com> wrote:

> On 18.04.22 09:35, Richard Guo wrote:
> > The array sortgrouprefs[] inside PathTarget might be NULL if we have not
> > identified sort/group columns in this tlist. In that case we would have
> > a NULL pointer reference in _outPathTarget() when trying to print
> > sortgrouprefs[] with WRITE_INDEX_ARRAY as we are using the length of
> > PathTarget->exprs as its array length.
>
> Do you have a test case that triggers this issue?
>

I don't have a test case. :(  I triggered this issue while debugging
with gdb and I was printing a certain 'pathlist' with nodeToString().

If it helps, here is the backtrace:

#0  in _outPathTarget (str=0x7fff683d7e50, node=0x56011e5cece0) at
outfuncs.c:2672
#1  in outNode (str=0x7fff683d7e50, obj=0x56011e5cece0) at outfuncs.c:4490
#2  in _outPathInfo (str=0x7fff683d7e50, node=0x56011e5f3408) at
outfuncs.c:1922
#3  in _outPath (str=0x7fff683d7e50, node=0x56011e5f3408) at outfuncs.c:1957
#4  in outNode (str=0x7fff683d7e50, obj=0x56011e5f3408) at outfuncs.c:4358
#5  in _outProjectionPath (str=0x7fff683d7e50, node=0x56011e5f3890) at
outfuncs.c:2154
#6  in outNode (str=0x7fff683d7e50, obj=0x56011e5f3890) at outfuncs.c:4409
#7  in _outAggPath (str=0x7fff683d7e50, node=0x56011e5f4550) at
outfuncs.c:2224
#8  in outNode (str=0x7fff683d7e50, obj=0x56011e5f4550) at outfuncs.c:4427
#9  in _outGatherPath (str=0x7fff683d7e50, node=0x56011e5f45e8) at
outfuncs.c:2142
#10 in outNode (str=0x7fff683d7e50, obj=0x56011e5f45e8) at outfuncs.c:4406
#11 in _outList (str=0x7fff683d7e50, node=0x56011e5f4680) at outfuncs.c:227
#12 in outNode (str=0x7fff683d7e50, obj=0x56011e5f4680) at outfuncs.c:4028
#13 in nodeToString (obj=0x56011e5f4680) at outfuncs.c:4782


Thanks
Richard

  9. Re: Fix NULL pointer reference in _outPathTarget()

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-04-22T14:16:02Z

    On 20.04.22 18:53, Tom Lane wrote:
>> I think we could put the if (node->fldname) inside the WRITE_INDEX_ARRAY
>> macro.
> 
> Yeah, that's another way to do it.  I think though that the unresolved
> question is whether or not we want the field name to appear in the output
> when the field is null.  I believe that I intentionally made it not appear
> originally, so that that case could readily be distinguished.  You could
> argue that that would complicate life greatly for a _readPathTarget()
> function, which is true, but I don't foresee that we'll need one.

We could adapt the convention to print NULL values as "<>", like

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 6a02f81ad5..4eb5be3787 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -127,8 +127,11 @@ static void outChar(StringInfo str, char c);
  #define WRITE_INDEX_ARRAY(fldname, len) \
     do { \
         appendStringInfoString(str, " :" CppAsString(fldname) " "); \
-       for (int i = 0; i < len; i++) \
-           appendStringInfo(str, " %u", node->fldname[i]); \
+       if (node->fldname) \
+           for (int i = 0; i < len; i++) \
+               appendStringInfo(str, " %u", node->fldname[i]); \
+       else \
+           appendStringInfoString(str, "<>"); \
     } while(0)

  #define WRITE_INT_ARRAY(fldname, len) \

There is currently no read function for this that would need to be 
changed.  But looking at peers such as WRITE_INT_ARRAY/READ_INT_ARRAY it 
shouldn't be hard to sort out if it became necessary.

  10. Re: Fix NULL pointer reference in _outPathTarget()

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-22T14:18:00Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 20.04.22 18:53, Tom Lane wrote:
>> Yeah, that's another way to do it.  I think though that the unresolved
>> question is whether or not we want the field name to appear in the output
>> when the field is null.  I believe that I intentionally made it not appear
>> originally, so that that case could readily be distinguished.  You could
>> argue that that would complicate life greatly for a _readPathTarget()
>> function, which is true, but I don't foresee that we'll need one.

> We could adapt the convention to print NULL values as "<>", like

Works for me.

			regards, tom lane

  11. Re: Fix NULL pointer reference in _outPathTarget()

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-04-27T07:17:35Z

    On 22.04.22 16:18, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>> On 20.04.22 18:53, Tom Lane wrote:
>>> Yeah, that's another way to do it.  I think though that the unresolved
>>> question is whether or not we want the field name to appear in the output
>>> when the field is null.  I believe that I intentionally made it not appear
>>> originally, so that that case could readily be distinguished.  You could
>>> argue that that would complicate life greatly for a _readPathTarget()
>>> function, which is true, but I don't foresee that we'll need one.
> 
>> We could adapt the convention to print NULL values as "<>", like
> 
> Works for me.

done