Re: Commit 86dc90056 - Rework planning and execution of UPDATE and DELETE

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Amit Langote <amitlangote09(at)gmail(dot)com>
Subject: Re: Commit 86dc90056 - Rework planning and execution of UPDATE and DELETE
Date: 2021-04-19 15:07:58
Message-ID: 223770.1618844878@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com> writes:
>> With the commit mentioned in the $subject, I am seeing the
>> change in behaviour with the varlena header size.

> Interesting. AFAICS, the new behavior is correct and the old is wrong.
> SET STORAGE PLAIN is supposed to disable use of TOAST features, including
> short varlena headers. So now that's being honored by the UPDATE, but
> before it was not. I have no idea exactly why that changed though ---
> I'd expect that to be implemented in low-level tuple-construction logic
> that the planner rewrite wouldn't have changed.

Oh, after a bit of tracing I see it. In v13, the new value gets
short-header-ified when a tuple is constructed here:

/*
* Ensure input tuple is the right format for the target relation.
*/
if (node->mt_scans[node->mt_whichplan]->tts_ops != planSlot->tts_ops)
{
ExecCopySlot(node->mt_scans[node->mt_whichplan], planSlot);
planSlot = node->mt_scans[node->mt_whichplan];
}

where the target slot has been made like this:

mtstate->mt_scans[i] =
ExecInitExtraTupleSlot(mtstate->ps.state, ExecGetResultType(mtstate->mt_plans[i]),
table_slot_callbacks(resultRelInfo->ri_RelationDesc));

So that's using a tupdesc that's been constructed according to the
default properties of the column datatypes, in particular attstorage
will be 'x' for the 'd' column. Later we transpose the data into
a slot that actually has the target table's rowtype, but the damage
is already done; the value isn't un-short-headerized at that point.
(I wonder if that should be considered a bug?)

86dc90056 got rid of the intermediate mt_scans slots, so the 'ab'
value only gets put into a slot that has the table's real descriptor,
and it never loses its original 4-byte header.

I observe that the INSERT code path still does the wrong thing:

regression=# insert into test_storage_char values('foo');
INSERT 0 1
regression=# SELECT d, pg_column_size(d) FROM test_storage_char;
d | pg_column_size
----------------------+----------------
ab | 24
foo | 21
(2 rows)

Maybe we oughta try to fix that sometime. It doesn't seem terribly
high-priority though.

regards, tom lane

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Yulin PEI 2021-04-19 15:19:29 回覆: 回复: Core dump happens when execute sql CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10));
Previous Message Justin Pryzby 2021-04-19 14:55:45 Re: track_planning causing performance regression