Re: generic plans and "initial" pruning

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, David Rowley *EXTERN* <dgrowleyml(at)gmail(dot)com>
Subject: Re: generic plans and "initial" pruning
Date: 2022-04-03 11:21:56
Message-ID: 202204031121.75frs4jhk5fc@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I noticed a definitional problem in 0001 that's also a bug in some
conditions -- namely that the bitmapset "validplans" is never explicitly
initialized to NIL. In the original coding, the BMS was always returned
from somewhere; in the new code, it is passed from an uninitialized
stack variable into the new ExecInitPartitionPruning function, which
then proceeds to add new members to it without initializing it first.
Indeed that function's header comment explicitly indicates that it is
not initialized:

+ * Initial pruning can be done immediately, so it is done here if needed and
+ * the set of surviving partition subplans' indexes are added to the output
+ * parameter *initially_valid_subplans.

even though this is not fully correct, because when prunestate->do_initial_prune
is false, then the BMS *is* initialized.

I have no opinion on where to initialize it, but it needs to be done
somewhere and the comment needs to agree.

I think the names ExecCreatePartitionPruneState and
ExecInitPartitionPruning are too confusingly similar. Maybe the former
should be renamed to somehow make it clear that it is a subroutine for
the former.

At the top of the file, there's a new comment that reads:

* ExecInitPartitionPruning:
* Creates the PartitionPruneState required by each of the two pruning
* functions.

What are "the two pruning functions"? I think here you mean "Append"
and "MergeAppend". Maybe spell that out explicitly.

I think this comment needs to be reworded:

+ * Subplans would previously be indexed 0..(n_total_subplans - 1) should be
+ * changed to index range 0..num(initially_valid_subplans).

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2022-04-03 11:34:00 Re: Higher level questions around shared memory stats
Previous Message Etsuro Fujita 2022-04-03 10:29:11 Re: Defer selection of asynchronous subplans until the executor initialization stage