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.