REPACK (CONCURRENTLY) might keep dropped-column data

Aha, so on my way to office I started thinking and got more silly ideas,
and now can confirm this is more widespread than logical subscriber use
case.

There's a case for BEFORE UPDATE trigger that might RETURN OLD. I.e. the
cases

-- 1
BEGIN RETURN OLD; END

-- 2
BEGIN NEW := OLD; RETURN NEW; END

-- 3
BEGIN OLD.a := NEW.a; RETURN OLD; END

are all affected by the same problem

CREATE TRIGGER t BEFORE UPDATE ON demo
FOR EACH ROW EXECUTE FUNCTION trg_return_old();

Confirmed by a single run

before 2288 kB
REPACK 72 kB
CONCURRENTLY, no updates 128 kB
CONCURRENTLY, rows updated (trigger) 2360 kB

How to replicate:
1. create table demo(id int primary key, a text, b text), fill it with
random data in b
2. add a BEFORE UPDATE trigger that does RETURN OLD
3. drop column b
4. keep updating rows while running REPACK (CONCURRENTLY) demo

Radim

On Wed, 30 Sept 2026 at 08:27, Radim Marek <radim@boringsql.com> wrote:

Hello Radim, thanks for testing!

On 2026-Sep-30, Radim Marek wrote:

Aha, so on my way to office I started thinking and got more silly ideas,
and now can confirm this is more widespread than logical subscriber use
case.

Oh, thanks for the simplified test case. We can fix this easily by
setting the column to null in the tuple to write out, as in the attached
patch. The adjust_toast_pointers() function should perhaps be renamed,
and the comment rewritten, since it's no longer just about toast ...
I didn't do that though.

I put together a crude test case to verify with isolationtester.
Without the fix, this reproduces the bloat you saw; with the fix, the
toast table's size after the repack is zero. This needs some more
boiling before being committable, but it suffices to show the problem.

Regards

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"The Gord often wonders why people threaten never to come back after they've
been told never to return" (www.actsofgord.com)

Álvaro Herrera <alvherre@kurilemu.de> wrote:

Hello Radim, thanks for testing!

On 2026-Sep-30, Radim Marek wrote:

Aha, so on my way to office I started thinking and got more silly ideas,
and now can confirm this is more widespread than logical subscriber use
case.

Oh, thanks for the simplified test case. We can fix this easily by
setting the column to null in the tuple to write out, as in the attached
patch.

I thought of fixing this on the decoding worker side so that the dropped
attribute values are not even written to the output file. However that would
require one more forming of the tuple.

The adjust_toast_pointers() function should perhaps be renamed,
and the comment rewritten, since it's no longer just about toast ...
I didn't do that though.

Maybe prepare_concurrent_update(), as it's called right before
apply_concurrent_update()?

BTW, I've noticed now that the 'relation' argument of adjust_toast_pointers()
isn't used anymore. Perhaps it was used before the tuple slots have been
introduced into the function.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com

Ok, I can't add much to the implementation discussion, but I can confirm
the patch resolves both use cases I reported.

Radim

On Wed, 30 Sept 2026 at 16:41, Antonin Houska <ah@cybertec.at> wrote:

On 2026-Sep-30, Radim Marek wrote:

Ok, I can't add much to the implementation discussion, but I can confirm
the patch resolves both use cases I reported.

Thank you, I have pushed it to both branches after adjusting the test
case a bit more.

I also noticed a problem in the implementation. We originally did this:

CompactAttribute *attr = TupleDescCompactAttr(desc, i);
varlena *varlena_dst;

if (attr->attisdropped)
+ {
+ dest->tts_isnull[i] = true;
continue;
+ }
if (attr->attlen != -1)
continue;
if (slot_attisnull(dest, i + 1))
continue;

slot_getsomeattrs(dest, i + 1);

Notice the slot_getsomeattrs() at the bottom: that is saying that if
the slot has not yet been deformed up to this attribute, then a
subsequent pass over the loop might overwrite the change of
dest->tts_isnull[] we did for this attribute! In order to do this
correctly, we must ensure that the slot has been deformed up to that
point, and _then_ we can modify tts_isnull. So I added another
slot_getsomeattrs() call inside the attisdropped() block. But, really,
the case where tts_isnull is already true for dropped columns is by far
the most common; it would be a shame to have to deform a bunch of
attributes only for there to be nothing to do. So I threw in a test
that the attribute is not already marked as null.

        if (attr->attisdropped)
+       {
+           if (!slot_attisnull(dest, i + 1))
+           {
+               slot_getsomeattrs(dest, i + 1);
+               dest->tts_isnull[i] = true;
+           }
            continue;
+       }

This way, we don't waste work.

(In practice, I don't expect this to have any effect, because the slot
uses the Virtual tts_ops, so it doesn't require deforming. But better
to do things by the book just in case.)

Thanks,

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"

On 2026-Sep-30, Antonin Houska wrote:

Álvaro Herrera <alvherre@kurilemu.de> wrote:

The adjust_toast_pointers() function should perhaps be renamed,
and the comment rewritten, since it's no longer just about toast ...
I didn't do that though.

Maybe prepare_concurrent_update(), as it's called right before
apply_concurrent_update()?

Thanks, I used this name.

BTW, I've noticed now that the 'relation' argument of adjust_toast_pointers()
isn't used anymore. Perhaps it was used before the tuple slots have been
introduced into the function.

Good catch. Removed. Yes, it was used in 0005 with v36 you submitted
[1]: /messages/by-id/87648.1772217509@localhost
it to use tuple slots.

[1]: /messages/by-id/87648.1772217509@localhost
[2]: /messages/by-id/202603191855.fzsgsnyzfvpt@alvherre.pgsql

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"El número de instalaciones de UNIX se ha elevado a 10,
y se espera que este número aumente" (UPM, 1972)

On Sat, Oct 3, 2026 at 8:35 PM Alvaro Herrera <alvherre@kurilemu.de> wrote:

On 2026-Sep-30, Radim Marek wrote:

Ok, I can't add much to the implementation discussion, but I can confirm
the patch resolves both use cases I reported.

Thank you, I have pushed it to both branches after adjusting the test
case a bit more.

Thanks for working on this!

'repack_decode',
+ 'repack_dropped',
'repack_missingval',

repack_dropped isolation test should also be listed in Makefile, in
addition to meson.build?

+teardown {
+ DROP TABLE repack_dropped;
+ DROP FUNCTION repack_dropped_f;
+}

Should teardown also drop the injection_points extension created by
setup, as the other tests that create the injection_points extension
do?

This missing DROP seems harmless with the current Meson test order, since
repack_missingval runs next, uses CREATE EXTENSION IF NOT EXISTS, and
then drops the extension. However, that seems fragile.

The attached patch fixes both issues.

Regards,

--
Fujii Masao

Hello,

On 2026-Oct-05, Fujii Masao wrote:

repack_dropped isolation test should also be listed in Makefile, in
addition to meson.build?

Oh, right.

+teardown {
+ DROP TABLE repack_dropped;
+ DROP FUNCTION repack_dropped_f;
+}

Should teardown also drop the injection_points extension created by
setup, as the other tests that create the injection_points extension
do?

This missing DROP seems harmless with the current Meson test order, since
repack_missingval runs next, uses CREATE EXTENSION IF NOT EXISTS, and
then drops the extension. However, that seems fragile.

Hmm, agreed, let's do this also.

I think the repeated creating/dropping is a bit wasteful though and we
don't need it. Wouldn't it make more sense to have all tests use CREATE
IF NOT EXISTS and then remove all the DROPs of it? We would end up
having the extension after the tests are run, but since the database is
specifically created to run the isolation tests, it's not a problem if
it remains there. The attached patch would do that. (I am proposing
this change just for pg20.)

Thanks!

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

Hi Alvaro,

A replayed INSERT can carry a dropped column's value too, and that
path still stores it.

A BEFORE INSERT trigger that returns a copy of
an existing row does it:

r := (SELECT t FROM demo t WHERE id = 1);
r.id := NEW.id;
RETURN r;

So does an UPDATE that moves the row to another partition while the
trigger returns OLD. COPY, MERGE and INSERT ON CONFLICT go the same
way.

It can also make REPACK fail. If the columns that are left need no
TOAST table, the new heap has none, and on master I get:

ERROR: row is too big: size 16424, maximum size 8160

0001 clears dropped columns in restore_tuple(), so every kind of
change is covered, and the block added to prepare_concurrent_update()
is no longer needed. 0002 adds a concurrent INSERT to repack_dropped.

I kept it apart in case you want only the fix.

Thanks,
Shihao

On Mon, Oct 5, 2026 at 8:35 PM Alvaro Herrera <alvherre@kurilemu.de> wrote:

I think the repeated creating/dropping is a bit wasteful though and we
don't need it. Wouldn't it make more sense to have all tests use CREATE
IF NOT EXISTS and then remove all the DROPs of it? We would end up
having the extension after the tests are run, but since the database is
specifically created to run the isolation tests, it's not a problem if
it remains there. The attached patch would do that. (I am proposing
this change just for pg20.)

+1

-# A foreign key check racing a rebuild of the index it resolves through.
#
# Pause the RI fast path before it locks the referenced table, allowing

Removing the first line of the comment seems unnecessary?

Regards,

--
Fujii Masao

On Mon, Oct 5, 2026 at 8:35 PM Alvaro Herrera <alvherre@kurilemu.de> wrote:

Hmm, agreed, let's do this also.

I've pushed the patch. Thanks!

Regards,

--
Fujii Masao

shihao zhong <zhong950419@gmail.com> wrote:

A replayed INSERT can carry a dropped column's value too, and that
path still stores it.

A BEFORE INSERT trigger that returns a copy of
an existing row does it:

r := (SELECT t FROM demo t WHERE id = 1);
r.id := NEW.id;
RETURN r;

So does an UPDATE that moves the row to another partition while the
trigger returns OLD. COPY, MERGE and INSERT ON CONFLICT go the same
way.

It can also make REPACK fail. If the columns that are left need no
TOAST table, the new heap has none, and on master I get:

ERROR: row is too big: size 16424, maximum size 8160

Can you please be more specific about this? I don't understand why the new
heap has no TOAST relation in such a case. Perhaps a bug in see
heapam_relation_needs_toast_table()? I'm not sure it should return here,
regardless the tuple size.

if (!has_toastable_attrs)
return false; /* nothing to toast? */

0001 clears dropped columns in restore_tuple(), so every kind of
change is covered, and the block added to prepare_concurrent_update()
is no longer needed.

Is this needed even for DELETE?

--
Antonin Houska
Web: https://www.cybertec-postgresql.com

Antonin Houska <ah@cybertec.at> wrote:

shihao zhong <zhong950419@gmail.com> wrote:

It can also make REPACK fail. If the columns that are left need no
TOAST table, the new heap has none, and on master I get:

ERROR: row is too big: size 16424, maximum size 8160

Can you please be more specific about this? I don't understand why the new
heap has no TOAST relation in such a case. Perhaps a bug in see
heapam_relation_needs_toast_table()? I'm not sure it should return here,
regardless the tuple size.

if (!has_toastable_attrs)
return false; /* nothing to toast? */

I could reproduce error like this independently from REPACK - see the scripts
attached. However, I realize that it does not help to remove this part from
heapam_relation_needs_toast_table(): it would only enforce creation of the
TOAST table if all the attributes had storage TYPSTORAGE_PLAIN, but such
attributes cannot be TOASTed anyway.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com

Hi Antonin,

Can you please be more specific about this? I don't understand why the new
heap has no TOAST relation in such a case.

heapam_relation_needs_toast_table() skips dropped columns. In my test
the dropped column was the only varlena one, the rest are ints. So
the old heap still has its TOAST table, and the new heap gets none:

CREATE TABLE rd (id int PRIMARY KEY, n int, b text);
ALTER TABLE rd DROP COLUMN b;
REPACK rd;
SELECT reltoastrelid FROM pg_class WHERE relname = 'rd'; -- 0

I think the function is right. The copy never carries a dropped
column's value over, so the new heap has nothing to toast. Only a
replayed change can bring one in, and then it has to fit in the page.

Is this needed even for DELETE?

No.

In your script the live columns are too wide, so nothing can help.
Here the value belongs to a dropped column, and REPACK can drop it.

v2 clears the columns only when replaying an INSERT. restore_tuple()
and prepare_concurrent_update() stay as they are.

Thanks,
Shihao

On 2026-Oct-06, shihao zhong wrote:

v2 clears the columns only when replaying an INSERT. restore_tuple()
and prepare_concurrent_update() stay as they are.

Hm, I just committed v1 with some trivial changes. I don't understand
what does v2 achieve. Are you trying to save having to scan the
attribute array for tuples in DELETE?

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

Hi Alvaro,

Thanks for committing it.

Are you trying to save having to scan the attribute array for tuples
in DELETE?

Yes. Antonin asked if the clearing is needed for DELETE. It is not,
since the old tuple is only used to find the row. But the loop is
cheap, and one place for all changes is simpler. So I withdraw v2.

Thanks,
Shihao