Hi Soumyadeep, On Wed, Jul 8, 2020 at 9:37 AM Soumyadeep Chakraborty <soumyadeep2...@gmail.com> wrote: > On Tue, Jul 7, 2020 at 7:18 AM Amit Langote <amitlangot...@gmail.com> wrote: > > If I change this to return a "heap" slot for partitioned tables, just > > like for foreign tables, the problem goes away (see the attached). In > > fact, even make check-world passes, so I don't know why it isn't that > > way to begin with. > > This is what I had thought of initially but I had taken a step back for 2 > reasons: > > 1. It is not mandatory for an AM to supply a heap tuple in the slot > passed to any AM routine. With your patch, the slot passed to > table_tuple_insert() inside ExecInsert() for instance is now expected to > supply a heap tuple for the subsequent call to ExecProcessReturning(). > This can lead to garbage values for xmin, xmax, cmin and cmax. I tried > applying your patch on Zedstore [1], a columnar AM, and consequently, I > got incorrect values for xmin, xmax etc with the query reported in this > issue.
Ah, I see. You might've noticed that ExecInsert() only ever sees leaf partitions, because tuple routing would've switched the result relation to a leaf partition by the time we are in ExecInsert(). So, table_tuple_insert() always refers to a leaf partition's AM. Not sure if you've also noticed but each leaf partition gets to own a slot (PartitionRoutingInfo.pi_PartitionTupleSlot), but currently it is only used if the leaf partition attribute numbers are not the same as the root partitioned table. How about we also use it if the leaf partition AM's table_slot_callbacks() differs from the root partitioned table's slot's tts_ops? That would be the case, for example, if the leaf partition is of Zedstore AM. In the more common cases where all leaf partitions are of heap AM, this would mean the original slot would be used as is, that is, if we accept hard-coding table_slot_callbacks() to return a "heap" slot for partitioned tables as I suggest. -- Amit Langote EnterpriseDB: http://www.enterprisedb.com
