Re: BUG #18830: ExecInitMerge Segfault on MERGE

On Thu, Mar 13, 2025 at 4:23 AM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On Wed, 12 Mar 2025 at 16:11, Tender Wang <tndrwang(at)gmail(dot)com> wrote:
> >
> > I can find another same error in the below query:
> >
> > with t as (update part_abc set c = true where a = stable_one() +2 returning a) insert into foo select a from t;
> >

Yes, I missed that PlannedStmt.resultRelations can include entries
from multiple ModifyTable nodes -- not the first time I’ve overlooked
that. :(

> Interesting example. That passes in HEAD, but fails with the v1 patch
> because it is attempting to open a pruned result relation when it
> doesn't need to.
>
> Attached is a rough patch that attempts to solve these issues locally
> in ExecInitModifyTable(), by not allowing all result relations to be
> pruned for that node. That makes it easy to handle cases with multiple
> ModifyTable nodes.

Thanks for the patch. I have some comments on the approach.

> This also has the advantage that all these relations can continue to
> be pruned from the subplans of the ModifyTable nodes, making those
> scans more efficient, and it only keeps a pruned result relation if
> all other result relations have been pruned.
>
> It does mean that ExecGetRangeTableRelation() needs to allow pruned
> relations to be opened, if called from ExecInitResultRelation(). I
> think that's OK because the check for opening pruned relations still
> applies to scan relations.

+ if (isResultRel && pruned)
+ {
+ /*
+ * A pruned result relation might not have been locked yet, so we
+ * must lock it now.
+ */
+ rel = table_open(rte->relid, rte->rellockmode);
+ }

One thing I'd like to avoid is taking any locks during ExecInitNode(),
including in functions like ExecGetRangeTableRelation(). Doing so
would require checking whether the CachedPlan is still valid and
handling invalidation if it isn't -- see commit 525392d5727f for
background.

Handling that correctly would mean aborting plan initialization,
cleaning up any partially initialized nodes, and returning early from
ExecInitNode(). To avoid that complexity, I've arranged for all
remaining necessary locks -- specifically on leaf partitions that
survive pruning -- to be taken in ExecDoInitialPruning(), which is
invoked by InitPlan() before any plan tree is initialized, and
checking CachedPlan validity once after returning from it:

/* ----------------------------------------------------------------
* InitPlan
*
* Initializes the query plan: open files, allocate storage
* and start up the rule manager
*
* If the plan originates from a CachedPlan (given in queryDesc->cplan),
* it can become invalid during runtime "initial" pruning when the
* remaining set of locks is taken. The function returns early in that
* case without initializing the plan, and the caller is expected to
* retry with a new valid plan.
* ----------------------------------------------------------------
*/
static void
InitPlan(QueryDesc *queryDesc, int eflags)
{
...
ExecDoInitialPruning(estate);

if (!ExecPlanStillValid(estate))
return;

So I'd prefer to keep all locking outside of ExecInitNode() for that reason.

One way to deal with multiple ModifyTable nodes is to have PlannedStmt
remember the "first" result relation of each, either as a List or a
Bitmapset. But I’m not sure Tom would be on board with that,
considering the work we did around 2018, e.g., commit 52ed730d511.
Also, adding a new field to PlannedStmt just to support a corner case
like this might be overkill -- though I’m fine with it if there’s no
other non-invasive way to address the issue. The alternative you
mentioned earlier -- modifying the code to use
ModifyTableState.rootResultRelInfo instead of resultRelInfo[0] --
seems at least somewhat invasive.

I’ve attached v3, which implements the above approach. It also
includes your changes to explain.c, the ExecGetRangeTableRelation()
interface adjustment, the Assert fixes in ExecInitModifyTable(), and
the test cases.

> I also adjusted explain.c slightly, to avoid the dummy pruned result
> relation that we might now keep from appearing in the EXPLAIN output
> as a target relation. Allowing it to be shown looked a bit odd,
> because it's not really the target relation in any real sense.

I agree about this part.

> I also noticed a few Asserts in ExecInitModifyTable() that didn't
> appear to be doing what they were originally intended to do.

Check too.

--
Thanks, Amit Langote