Re: posgres 12 bug (partitioned table)

Hi,

On Wed, Aug 12, 2020 at 3:02 AM Andres Freund <andres@anarazel.de> wrote:
> On 2020-06-04 12:57:30 -0400, Tom Lane wrote:
> > Pavel Biryukov <79166341370@yandex.ru> writes:
> > > Hello. I'm catch error "virtual tuple table slot does not have system
> > > attributes" when inserting row into partitioned table with RETURNING
> > > xmin
> >
> > Reproduced here.  The example works in v11, and in v10 if you remove
> > the unnecessary-to-the-example primary key, so it seems like a clear
> > regression.  I didn't dig for the cause but I suppose it's related
> > to Andres' slot-related changes.
>
> The reason we're getting the failure is that nodeModifyTable.c only is
> dealing with virtual tuple slots, which don't carry visibility
> information. Despite actually having infrastructure for creating a
> partition specific slot.  If I force check_attrmap_match() to return
> false, the example starts working.
>
> I don't really know how to best fix this in the partitioning
> infrastructure. Currently the determination whether there needs to be
> any conversion between subplan slot and the slot used for insertion is
> solely based on comparing tuple descriptors. But for better or worse, we
> don't represent system column accesses in tuple descriptors.
>
> It's not that hard to force the slot creation & use whenever there's
> returning, but somehow that feels hackish (but so does plenty other
> things in execPartition.c).  See attached.
>
> But I'm worried that that's not enough: What if somebody in a trigger
> wants to access system columns besides tableoid and tid (which are
> handled in a generic manner)? Currently - only for partitioned table DML
> going through the root table - we'll not have valid values for the
> trigger. It's pretty dubious imo to use xmin/xmax in triggers, but ...
>
> I suspect we should just unconditionally use
> partrouteinfo->pi_PartitionTupleSlot. Going through
> partrouteinfo->pi_RootToPartitionMap if present, and ExecCopySlot()
> otherwise.

I see that to be the only way forward even though there will be a
slight hit in performance in typical cases where a virtual tuple slot
suffices.

> Medium term I think we should just plain out forbid references to system
> columns in partioned tables Or at least insist that all partitions have
> that column.

Performance-wise I would prefer the former, because the latter would
involve checking *all* partitions statically in the INSERT case,
something that we've avoided doing so far.

-- 
Amit Langote
EnterpriseDB: http://www.enterprisedb.com