agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedREPACK (CONCURRENTLY) might keep dropped-column data
7+ messages / 4 participants
[nested] [flat]
* REPACK (CONCURRENTLY) might keep dropped-column data
@ 2026-09-30 06:27 Radim Marek <radim@boringsql.com>
0 siblings, 1 reply; 7+ messages in thread
From: Radim Marek @ 2026-09-30 06:27 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; +Cc: ah@cybertec.at <ah@cybertec.at>
Hello,
last night I found my small issue as one of REPACK (CONCURRENTLY) testing.
It's similar to an old problem with pg_squeeze reported to Antonin some
time ago.
I played with the idea how it might cope under the logical replication (on
subscriber) and given the previous experience with dropped columns, I
managed to hit scenario where it leaves old data behind.
While the initial copy removes the dropped column data as expected, any
changes that don't follow regular UPDATE path seems to retain the old value.
The table sizes shows the problem nicely
before 2424 kB
REPACK 224 kB
CONCURRENTLY, no replicated updates 288 kB
CONCURRENTLY, rows updated 2616 kB
The local apply worker seems to build the data from the original tuple,
without setting dropped value to NULL.
How to replicate:
1. On publisher create table demo(id int primary key, a text)
2. On subscriber set the table demo(id int primary key, a text, b text)
3. Populate values on publishers, set random data in b on subscriber
4. Drop the column 'b' on subscriber
5. Keep updating data on publisher
6. run REPACK (CONCURRENTLY) on subscriber table
Hope this helps. I will try to look more into the source of the problem
once I have time (if needed).
Radim
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
@ 2026-09-30 07:37 Radim Marek <radim@boringsql.com>
parent: Radim Marek <radim@boringsql.com>
0 siblings, 1 reply; 7+ messages in thread
From: Radim Marek @ 2026-09-30 07:37 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; +Cc: ah@cybertec.at <ah@cybertec.at>
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,
>
> last night I found my small issue as one of REPACK (CONCURRENTLY) testing.
> It's similar to an old problem with pg_squeeze reported to Antonin some
> time ago.
>
> I played with the idea how it might cope under the logical replication (on
> subscriber) and given the previous experience with dropped columns, I
> managed to hit scenario where it leaves old data behind.
>
> While the initial copy removes the dropped column data as expected, any
> changes that don't follow regular UPDATE path seems to retain the old value.
>
> The table sizes shows the problem nicely
>
> before 2424 kB
> REPACK 224 kB
> CONCURRENTLY, no replicated updates 288 kB
> CONCURRENTLY, rows updated 2616 kB
>
> The local apply worker seems to build the data from the original tuple,
> without setting dropped value to NULL.
>
> How to replicate:
> 1. On publisher create table demo(id int primary key, a text)
> 2. On subscriber set the table demo(id int primary key, a text, b text)
> 3. Populate values on publishers, set random data in b on subscriber
> 4. Drop the column 'b' on subscriber
> 5. Keep updating data on publisher
> 6. run REPACK (CONCURRENTLY) on subscriber table
>
> Hope this helps. I will try to look more into the source of the problem
> once I have time (if needed).
>
> Radim
>
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
@ 2026-09-30 11:48 Álvaro Herrera <alvherre@kurilemu.de>
parent: Radim Marek <radim@boringsql.com>
0 siblings, 1 reply; 7+ messages in thread
From: Álvaro Herrera @ 2026-09-30 11:48 UTC (permalink / raw)
To: Radim Marek <radim@boringsql.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; ah@cybertec.at <ah@cybertec.at>
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)
Attachments:
[text/x-diff] 0001-Clear-out-values-from-dropped-columns.patch (1.3K, ../../arz09-sHiSl3qBN0@alvherre.pgsql/2-0001-Clear-out-values-from-dropped-columns.patch)
download | inline diff:
From 4ff5f220652e8a2bdb20ce3c6b3958ac7f6e312d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Wed, 30 Sep 2026 13:37:34 +0200
Subject: [PATCH 1/2] Clear out values from dropped columns
Reported-by: Radim Marek <radim@boringsql.com>
Discussion: https://postgr.es/m/CAJgoLkK2UBzB1J9buCsSUbjf7bOqz-o_0=CeiTruU789BhBw-Q@mail.gmail.com
---
src/backend/commands/repack.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 596c1abaf78..eadaec72d30 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -3017,6 +3017,9 @@ restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot)
/*
* Adjust 'dest' replacing any EXTERNAL_ONDISK toast pointers with the
* corresponding ones from 'src'.
+ *
+ * We also take the opportunity to clear out the values in columns that were
+ * dropped.
*/
static void
adjust_toast_pointers(Relation relation, TupleTableSlot *dest, TupleTableSlot *src)
@@ -3029,7 +3032,10 @@ adjust_toast_pointers(Relation relation, TupleTableSlot *dest, TupleTableSlot *s
varlena *varlena_dst;
if (attr->attisdropped)
+ {
+ dest->tts_isnull[i] = true;
continue;
+ }
if (attr->attlen != -1)
continue;
if (slot_attisnull(dest, i + 1))
--
2.47.3
[text/x-diff] 0002-Crude-test-case.patch (1.9K, ../../arz09-sHiSl3qBN0@alvherre.pgsql/3-0002-Crude-test-case.patch)
download | inline diff:
From d4a40603003fadd01456ad74f17c97a219c365f3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Wed, 30 Sep 2026 13:43:45 +0200
Subject: [PATCH 2/2] Crude test case
XXX not for commit just yet
---
.../specs/repack_dropped.spec | 52 +++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100644 src/test/modules/injection_points/specs/repack_dropped.spec
diff --git a/src/test/modules/injection_points/specs/repack_dropped.spec b/src/test/modules/injection_points/specs/repack_dropped.spec
new file mode 100644
index 00000000000..1a1ea085299
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_dropped.spec
@@ -0,0 +1,52 @@
+setup {
+ CREATE EXTENSION IF NOT EXISTS injection_points;
+
+ create table repack_dropped (id int primary key, a text, b text);
+ alter table repack_dropped alter column b set storage external;
+ insert into repack_dropped select g, cash_words(g::money), repeat(cash_words(g::money), 10 * g) from generate_series(10, 100) g;
+ create function repack_dropped_f() returns trigger language plpgsql as $$ begin return OLD; end $$;
+ create trigger repack_dropped_t before update on repack_dropped for each row execute function repack_dropped_f();
+ alter table repack_dropped drop column b;
+}
+
+teardown {
+ drop table repack_dropped;
+ drop function repack_dropped_f;
+}
+
+session s1
+
+step s1_size
+{
+ select pg_relation_size(oid), pg_relation_size(reltoastrelid) from pg_class where relname = 'repack_dropped';
+}
+
+step s1_unlock
+{
+ SELECT injection_points_wakeup('repack-concurrently-before-lock');
+}
+
+session s2
+setup
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('repack-concurrently-before-lock', 'wait');
+}
+
+step s2_repack
+{
+ repack (concurrently) repack_dropped;
+}
+
+session s3
+step s3_updates
+{
+ update repack_dropped set a = a || a;
+}
+
+permutation
+ s1_size
+ s2_repack
+ s3_updates
+ s1_unlock
+ s1_size
--
2.47.3
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
@ 2026-09-30 14:41 Antonin Houska <ah@cybertec.at>
parent: Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 2 replies; 7+ messages in thread
From: Antonin Houska @ 2026-09-30 14:41 UTC (permalink / raw)
To: alvherre@kurilemu.de; +Cc: Radim Marek <radim@boringsql.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Á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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
@ 2026-09-30 15:38 Radim Marek <radim@boringsql.com>
parent: Antonin Houska <ah@cybertec.at>
1 sibling, 1 reply; 7+ messages in thread
From: Radim Marek @ 2026-09-30 15:38 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: alvherre@kurilemu.de, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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:
> Á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
>
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
@ 2026-10-03 11:35 Alvaro Herrera <alvherre@kurilemu.de>
parent: Radim Marek <radim@boringsql.com>
0 siblings, 0 replies; 7+ messages in thread
From: Alvaro Herrera @ 2026-10-03 11:35 UTC (permalink / raw)
To: Radim Marek <radim@boringsql.com>; +Cc: Antonin Houska <ah@cybertec.at>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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"
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
@ 2026-10-03 13:48 Alvaro Herrera <alvherre@kurilemu.de>
parent: Antonin Houska <ah@cybertec.at>
1 sibling, 0 replies; 7+ messages in thread
From: Alvaro Herrera @ 2026-10-03 13:48 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Radim Marek <radim@boringsql.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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], but it disappeared when I posted v43 [2], which is when I rewrote
it to use tuple slots.
[1] https://www.postgresql.org/message-id/87648.1772217509@localhost
[2] https://www.postgresql.org/message-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)
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-10-03 13:48 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-09-30 06:27 REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 07:37 ` Radim Marek <radim@boringsql.com>
2026-09-30 11:48 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-09-30 14:41 ` Antonin Houska <ah@cybertec.at>
2026-09-30 15:38 ` Radim Marek <radim@boringsql.com>
2026-10-03 11:35 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-10-03 13:48 ` Alvaro Herrera <alvherre@kurilemu.de>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox