Re: [PATCH] Allow multiple recursive self-references

The explanations below are satisfactory to me.  I think the executor 
changes in this patch are ok.  But I would like someone else who has 
more experience in this particular area to check it too; I'm not going 
to take committer responsibility for this by myself without additional 
review.

As I said earlier, I think semantically/mathematically, the changes 
proposed by this patch set are okay.

The rewriting in the parse analysis is still being debated, but it can 
be tackled in separate patches/commits.


On 11.01.22 12:33, Denis Hirn wrote:
>> I wonder why in ExecWorkTableScan() and ExecReScanWorkTableScan() you call
>> tuplestore_copy_read_pointer() instead of just tuplestore_select_read_pointer().
> The WorkTableScan reads the working_table tuplestore of the parent RecursiveUnion
> operator. But after each iteration of the RecursiveUnion operator, the following
> operations are performed:
> 
>> 122    /* done with old working table ... */
>> 123    tuplestore_end(node->working_table);                              -- (1)
>> 124
>> 125    /* intermediate table becomes working table */
>> 126    node->working_table = node->intermediate_table;                   -- (2)
>> 127
>> 128      /* create new empty intermediate table */
>> 129      node->intermediate_table = tuplestore_begin_heap(false, false,
>> 130                                                       work_mem);     -- (3)
> https://doxygen.postgresql.org/nodeRecursiveunion_8c_source.html#l00122
> 
> In step (1), the current working_table is released. Therefore, all read pointers
> that we had additionally allocated are gone, too. The intermediate table becomes
> the working table in step (2), and finally a new empty intermediate table is
> created (3).
> 
> Because of step (1), we have to allocate new unique read pointers for each worktable
> scan again. Just using tuplestore_select_read_pointer() would be incorrect.
> 
>> What is the special role of read pointer 0 that you are copying.  Before your
>> changes, it was just the implicit read pointer, but now that we have several,
>> it would be good to explain their relationship.
> To allocate a new read pointer, tuplestore_alloc_read_pointer() could also be used.
> However, we would have to know about the eflags parameter – which the worktable
> scan has no information about.
> 
> The important thing about read pointer 0 is that it always exists, and it is initialized correctly.
> Therefore, it is save to copy read pointer 0 instead of creating a new one from scratch.
> 
> 
>> Also, the comment you deleted says "Therefore, we don't need a private read pointer
>> for the tuplestore, nor do we need to tell tuplestore_gettupleslot to copy."
>> You addressed the first part with the read pointer handling, but what about the
>> second part?  The tuplestore_gettupleslot() call in WorkTableScanNext() still
>> has copy=false.  Is this an oversight, or did you determine that copying is
>> still not necessary?
> That's an oversight. Copying is still not necessary. Copying would only be required,
> if additional writes to the tuplestore occur. But that can not happen here.