Horton

Re: generic plans and "initial" pruning

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Tender Wang <tndrwang@gmail.com>, Alexander Lakhin <exclusion@gmail.com>, Tomas Vondra <tomas@vondra.me>, Robert Haas <robertmhaas@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Andres Freund <andres@anarazel.de>, Daniel Gustafsson <daniel@yesql.se>, David Rowley <dgrowleyml@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Thom Brown <thom@linux.com>
Date: 2025-05-20T03:06:21Z
Lists: pgsql-hackers

Attachments

Amit Langote <amitlangote09@gmail.com> writes:
> Pushed after some tweaks to comments and the test case.

My attention was drawn to commit 525392d57 after observing that
Valgrind complained about a memory leak in some code that commit added
to BuildCachedPlan().  I tried to make sense of said code so I could
remove the leak, and eventually arrived at the attached patch, which
is part of a series of leak-fixing things hence the high sequence
number.

Unfortunately, the bad things I speculated about in the added comments
seem to be reality.  The second attached file is a test case that
triggers

TRAP: failed Assert("list_length(plan_list) == list_length(plan->stmt_list)"), File: "plancache.c", Line: 1259, PID: 602087

because it adds a DO ALSO rule that causes the rewriter to generate
more PlannedStmts than it did before.

This is quite awful, because it does more than simply break the klugy
(and undocumented) business about keeping the top-level List in a
different context.  What it means is that any outside code that is
busy iterating that List is very fundamentally broken: it's not clear
what List index it ought to resume at, except that "the one it was at"
is demonstrably incorrect.

I also don't really believe the (also undocumented) assumption that
such outside code is in between executions of PlannedStmts of the
List and hence can tolerate those being ripped out and replaced.
I have not attempted to build an example, because the one I have
seems sufficiently damning.  But I bet that a recursive function
could be constructed in such a way that an outer execution is
still in progress when an inner call triggers UpdateCachedPlan.

Another small problem (much more easily fixable than the above,
probably) is that summarily setting "plan->is_valid = true"
at the end is not okay.  We could already have received an
invalidation that should result in marking the plan stale.
(Holding locks on the tables involved is not sufficient to
prevent that, as there are other sources of inval events.)

It's possible that this code can be fixed, but I fear it's
going to involve some really fundamental redesign, which
probably shouldn't be happening after beta1.  I think there
is no alternative but to revert for v18.

			regards, tom lane

Commits

  1. Stamp 19beta1.

  2. Revert "Don't lock partitions pruned by initial pruning"

  3. Ensure first ModifyTable rel initialized if all are pruned

  4. Fix bug in cbc127917 to handle nested Append correctly

  5. Remove unstable test suite added by 525392d57

  6. Don't lock partitions pruned by initial pruning

  7. Fix an oversight in cbc127917 to handle MERGE correctly

  8. Track unpruned relids to avoid processing pruned relations

  9. Perform runtime initial pruning outside ExecInitNode()

  10. Move PartitionPruneInfo out of plan nodes into PlannedStmt

  11. Fix setrefs.c's failure to do expression processing on prune steps.

  12. Remove obsolete executor cleanup code

  13. Revert "Move PartitionPruneInfo out of plan nodes into PlannedStmt"

  14. Move PartitioPruneInfo out of plan nodes into PlannedStmt

  15. Refactor and cleanup runtime partition prune code a little

  16. Remove some unnecessary fields from Plan trees.

  17. Remove more redundant relation locking during executor startup.

  18. Shut down Gather's children before shutting down Gather itself.