Re: [PATCH] Allow multiple recursive self-references

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>
Cc: Denis Hirn <denis(dot)hirn(at)uni-tuebingen(dot)de>, vignesh C <vignesh21(at)gmail(dot)com>, Pantelis Theodosiou <ypercube(at)gmail(dot)com>, David Fetter <david(at)fetter(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Zhihong Yu <zyu(at)yugabyte(dot)com>
Subject: Re: [PATCH] Allow multiple recursive self-references
Date: 2022-04-23 15:14:11
Message-ID: 3883422.1650726851@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> As I said earlier, I think semantically/mathematically, the changes
> proposed by this patch set are okay.

I took a quick look at this patch because I wondered how it would
affect the SEARCH/CYCLE bug discussed at [1]. Doesn't it break
rewriteSearchAndCycle() completely? That code assumes (without a
lot of checking) that a recursive query is a UNION [ALL] of exactly
two SELECTs.

Some other random thoughts while I'm looking at it (not a full review):

* The patch removes this comment in WorkTableScanNext:

- * Note: we are also assuming that this node is the only reader of the
- * worktable. Therefore, we don't need a private read pointer for the
- * tuplestore, nor do we need to tell tuplestore_gettupleslot to copy.

I see that it deals with the private-read-pointer question, but I do not
see any changes addressing the point about copying tuples fetched from the
tuplestore. Perhaps there's no issue, but if not, a comment explaining
why not would be appropriate.

* The long comment added to checkWellFormedRecursion will be completely
destroyed by pgindent, but that's the least of its problems: it does
not explain either why we need a tree rotation or why that doesn't
break SQL semantics. The shorter comment in front of it needs
rewritten, too. And I'm not really convinced that the new loop
is certain to terminate.

* The chunk added to checkWellFormedSelectStmt is undercommented,
because of which I'm not convinced that it's right at all. Since
that's really the meat of this patch, it needs more attention.
And the new variable names are still impossibly confusing.

regards, tom lane

[1] https://www.postgresql.org/message-id/flat/17320-70e37868182512ab%40postgresql.org

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message David Christensen 2022-04-23 18:43:36 Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL
Previous Message Matthias van de Meent 2022-04-23 14:49:03 Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL