Date: Thu, 22 Apr 2021 11:31:48 -0700
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Amit Langote <amitlangote09@gmail.com>,
	David Steele <david@pgmasters.net>,
	Pavel Biryukov <79166341370@yandex.ru>,
	PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: posgres 12 bug (partitioned table)
Message-ID: <20210422183148.xsopy2qsym7a7app@alap3.anarazel.de>
References: <20200812162718.ycxrktzoe6rpdy5k@alap3.anarazel.de>
 <CA+HiwqEGgy8x5M2dotzH7HSd0v+byyJXGU8S+d5NFnrOBiTZDA@mail.gmail.com>
 <407111603806900@mail.yandex.ru>
 <CA+HiwqFjEim_RMCiaFxA4nUx00o3auXKPy-OsOcog+chg5K-zA@mail.gmail.com>
 <cd492dbd-a0ba-4790-bcf7-c4af2d2dc8d1@pgmasters.net>
 <CA+HiwqG7qhKJnFpZOqyrFngqcMzja+p=gssMK=73b+-BZffxqw@mail.gmail.com>
 <CA+HiwqHqnC=A9a+=F-k8J=JV-EKKt3vTgxq01=9af-0HE045NQ@mail.gmail.com>
 <793243.1619032209@sss.pgh.pa.us>
 <CA+HiwqGASb2n+fOKVgDUddzV-_vc6ZxRj9i5oWMWOw1VK0_c7g@mail.gmail.com>
 <1102101.1619115665@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1102101.1619115665@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/20210422183148.xsopy2qsym7a7app%40alap3.anarazel.de>
Precedence: bulk

Hi,

On 2021-04-22 14:21:05 -0400, Tom Lane wrote:
> Hm, we're not done with this.  Whatever you think of the merits of
> throwing an implementation-level error, what's actually happening
> in v12 and v13 in the wake of a71cfc56b is that an attempt to do
> "RETURNING xmin" works in a non-cross-partition UPDATE, but in
> a cross-partition UPDATE it dumps core.  What I'm seeing is that
> the result of the execute_attr_map_slot() call looks like
> 
> (gdb) p *(BufferHeapTupleTableSlot*) scanslot
> $3 = {base = {base = {type = T_TupleTableSlot, tts_flags = 8, tts_nvalid = 3, 
>       tts_ops = 0xa305c0 <TTSOpsBufferHeapTuple>, 
>       tts_tupleDescriptor = 0x7f1c7798f920, tts_values = 0x2077100, 
>       tts_isnull = 0x2075fb0, tts_mcxt = 0x2015ea0, tts_tid = {ip_blkid = {
>           bi_hi = 0, bi_lo = 0}, ip_posid = 3}, tts_tableOid = 37812}, 
>     tuple = 0x0, off = 0, tupdata = {t_len = 0, t_self = {ip_blkid = {
>           bi_hi = 0, bi_lo = 0}, ip_posid = 0}, t_tableOid = 0, 
>       t_data = 0x0}}, buffer = 0}
> 
> and since base.tuple is NULL, heap_getsysattr dies on either
> "Assert(tup)" or a null-pointer dereference.
> 
> So
> 
> (1) It seems like this is exposing a shortcoming in the
> multiple-slot-types logic.  It's not hard to understand why the slot would
> look like this after execute_attr_map_slot does ExecStoreVirtualTuple,
> but if this is not a legal state for a BufferHeapTuple slot, why didn't
> ExecStoreVirtualTuple raise a complaint?

I think it's too useful to support ExecStoreVirtualTuple() in a
heap[buffer] slot to disallow that. Seems like we should make
tts_buffer_heap_getsysattr() error out if there's no tuple?


> (2) It also seems like we can't use the srcSlot if we want to have
> the fail-because-its-a-virtual-tuple behavior.  I experimented with
> doing ExecMaterializeSlot on the result of execute_attr_map_slot,
> and that stops the crash, but then we're returning garbage values
> of xmin etc, which does not seem good.

Garbage values as in 0's, or random data? Seems like it should be the
former?

Greetings,

Andres Freund