Re: Eliminating SPI from RI triggers - take 2

On Wed, Oct 12, 2022 at 2:27 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Sep 29, 2022 at 12:47 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > [ patches ]
>
> While looking over this thread I came across this code:

Thanks for looking.

> /* For data reading, executor always omits detached partitions */
> if (estate->es_partition_directory == NULL)
> estate->es_partition_directory =
> CreatePartitionDirectory(estate->es_query_cxt, false);
>
> But CreatePartitionDirectory is declared like this:
>
> extern PartitionDirectory CreatePartitionDirectory(MemoryContext mcxt,
> bool omit_detached);
>
> So the comment seems to say the opposite of what the code does. The
> code seems to match the explanation in the commit message for
> 71f4c8c6f74ba021e55d35b1128d22fb8c6e1629, so I am guessing that
> perhaps s/always/never/ is needed here.

I think you are right. In commit 8aba9322511 that fixed a bug in this
area, we have this hunk:

- /* Executor must always include detached partitions */
+ /* For data reading, executor always omits detached partitions */
if (estate->es_partition_directory == NULL)
estate->es_partition_directory =
- CreatePartitionDirectory(estate->es_query_cxt, true);
+ CreatePartitionDirectory(estate->es_query_cxt, false);

The same commit also renamed the include_detached parameter of
CreatePartitionDirectory() to omit_detached but the comment change
didn't quite match with that.

I will fix this and other related comments to be consistent about
using the word "omit". Will include them in the updated 0003.

> I also noticed that ExecCreatePartitionPruneState no longer exists in
> the code but is still referenced in
> src/test/modules/delay_execution/specs/partition-addition.spec

It looks like we missed that reference in commit 297daa9d435 wherein
we renamed it to just CreatePartitionPruneState().

I have posted a patch to fix this.

> Regarding 0003, it seems unfortunate that
> find_inheritance_children_extended() will now have 6 arguments 4 of
> which have to do with detached partition handling. That is a lot of
> detached partition handling, and it's hard to reason about. I don't
> see an obvious way of simplifying things very much, but I wonder if we
> could at least have the new omit_detached_snapshot snapshot replace
> the existing bool omit_detached flag. Something like the attached
> incremental patch.

Yeah, I was wondering the same too and don't see a reason why we
couldn't do it that way.

I have merged your incremental patch into 0003.

> Probably we need to go further than the attached, though. I don't
> think that PartitionDirectoryLookup() should be getting any new
> arguments. The whole point of that function is that it's supposed to
> ensure that the returned value is stable, and the comments say so. But
> with these changes it isn't any more, because it depends on the
> snapshot you pass. It seems fine to specify when you create the
> partition directory that you want it to show a different, still-stable
> view of the world, but as written, it seems to me to undermine the
> idea that the return value is expected to be stable at all. Is there a
> way we can avoid that?

Ok, I think it makes sense to have CreatePartitionDirectory take in
the snapshot and store it in PartitionDirectoryData for use during
each subsequent PartitionDirectoryLookup(). So we'll be replacing the
current omit_detached flag in PartitionDirectoryData, just as we are
doing for the interface functions. Done that way in 0003.

Regarding 0002, which introduces ri_LookupKeyInPkRel(), I realized
that it may have been initializing the ScanKeys wrongly. It was using
ScanKeyInit(), which uses InvalidOid for sk_subtype, causing the index
AM / btree code to use the wrong comparison functions when PK and FK
column types don't match. That may have been a reason for 32-bit
machine failures pointed out by Andres upthread. I've fixed it by
using ScanKeyEntryInitialize() to pass the opfamily-specified right
argument (FK column) type OID.

Attached updated patches.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com