agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedREPACK (CONCURRENTLY) might keep dropped-column data
10+ messages / 6 participants
[nested] [flat]
* REPACK (CONCURRENTLY) might keep dropped-column data
@ 2026-09-30 06:27 Radim Marek <radim@boringsql.com>
2026-09-30 07:37 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
0 siblings, 1 reply; 10+ 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] 10+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
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 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 10+ 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] 10+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
2026-09-30 06:27 REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 07:37 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
@ 2026-09-30 11:48 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-09-30 14:41 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 10+ 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] 10+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
2026-09-30 06:27 REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 07:37 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 11:48 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Álvaro Herrera <alvherre@kurilemu.de>
@ 2026-09-30 14:41 ` Antonin Houska <ah@cybertec.at>
2026-09-30 15:38 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-10-03 13:48 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 2 replies; 10+ 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] 10+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
2026-09-30 06:27 REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 07:37 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 11:48 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Álvaro Herrera <alvherre@kurilemu.de>
2026-09-30 14:41 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Antonin Houska <ah@cybertec.at>
@ 2026-09-30 15:38 ` Radim Marek <radim@boringsql.com>
2026-10-03 11:35 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Alvaro Herrera <alvherre@kurilemu.de>
1 sibling, 1 reply; 10+ 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] 10+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
2026-09-30 06:27 REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 07:37 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 11:48 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Álvaro Herrera <alvherre@kurilemu.de>
2026-09-30 14:41 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Antonin Houska <ah@cybertec.at>
2026-09-30 15:38 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
@ 2026-10-03 11:35 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-10-05 03:51 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Fujii Masao <masao.fujii@gmail.com>
0 siblings, 1 reply; 10+ 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] 10+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
2026-09-30 06:27 REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 07:37 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 11:48 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Álvaro Herrera <alvherre@kurilemu.de>
2026-09-30 14:41 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Antonin Houska <ah@cybertec.at>
2026-09-30 15:38 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-10-03 11:35 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Alvaro Herrera <alvherre@kurilemu.de>
@ 2026-10-05 03:51 ` Fujii Masao <masao.fujii@gmail.com>
2026-10-05 11:35 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 10+ messages in thread
From: Fujii Masao @ 2026-10-05 03:51 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@kurilemu.de>; +Cc: Radim Marek <radim@boringsql.com>; Antonin Houska <ah@cybertec.at>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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
Attachments:
[application/octet-stream] v1-0001-Fix-registration-and-cleanup-of-repack_dropped-is.patch (1.3K, ../../CAHGQGwG9bHxhs4Nj+9avSjSGh0vdYNQgEuz+pmJ-H5be6tEBTw@mail.gmail.com/2-v1-0001-Fix-registration-and-cleanup-of-repack_dropped-is.patch)
download | inline diff:
From 816ef00fad99fd89f3977890cc526b00784b6776 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Mon, 5 Oct 2026 12:17:30 +0900
Subject: [PATCH v1] Fix registration and cleanup of repack_dropped isolation
test
---
src/test/modules/injection_points/Makefile | 1 +
src/test/modules/injection_points/specs/repack_dropped.spec | 1 +
2 files changed, 2 insertions(+)
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 1d7d4d66d11..2463d6e0fb9 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -25,6 +25,7 @@ ISOLATION = basic \
repack \
repack_commit_race \
repack_decode \
+ repack_dropped \
repack_missingval \
repack_temporal \
repack_temporal_multirange \
diff --git a/src/test/modules/injection_points/specs/repack_dropped.spec b/src/test/modules/injection_points/specs/repack_dropped.spec
index 099cf744202..878cdeffc5c 100644
--- a/src/test/modules/injection_points/specs/repack_dropped.spec
+++ b/src/test/modules/injection_points/specs/repack_dropped.spec
@@ -24,6 +24,7 @@ setup {
teardown {
DROP TABLE repack_dropped;
DROP FUNCTION repack_dropped_f;
+ DROP EXTENSION injection_points;
}
session s1
--
2.55.0
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
2026-09-30 06:27 REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 07:37 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 11:48 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Álvaro Herrera <alvherre@kurilemu.de>
2026-09-30 14:41 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Antonin Houska <ah@cybertec.at>
2026-09-30 15:38 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-10-03 11:35 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Alvaro Herrera <alvherre@kurilemu.de>
2026-10-05 03:51 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Fujii Masao <masao.fujii@gmail.com>
@ 2026-10-05 11:35 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-10-05 15:46 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data shihao zhong <zhong950419@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Alvaro Herrera @ 2026-10-05 11:35 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@gmail.com>; +Cc: Radim Marek <radim@boringsql.com>; Antonin Houska <ah@cybertec.at>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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/
Attachments:
[text/x-diff] cine-injpts.patch (11.5K, ../../asOEsmPw75ifqMua@alvherre.pgsql/2-cine-injpts.patch)
download | inline diff:
diff --git a/src/test/modules/injection_points/specs/basic.spec b/src/test/modules/injection_points/specs/basic.spec
index 13d2793f6e4..509572ae9ea 100644
--- a/src/test/modules/injection_points/specs/basic.spec
+++ b/src/test/modules/injection_points/specs/basic.spec
@@ -6,11 +6,7 @@
setup
{
- CREATE EXTENSION injection_points;
-}
-teardown
-{
- DROP EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
}
# Wait happens in the first session, wakeup in the second session.
diff --git a/src/test/modules/injection_points/specs/heap_lock_update.spec b/src/test/modules/injection_points/specs/heap_lock_update.spec
index b3992a1eb7a..6184ed9f5b7 100644
--- a/src/test/modules/injection_points/specs/heap_lock_update.spec
+++ b/src/test/modules/injection_points/specs/heap_lock_update.spec
@@ -22,7 +22,7 @@
# output, to verify that the test exercises the scenario we want.
setup
{
- CREATE EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
CREATE TABLE t (id int PRIMARY KEY);
do $$
@@ -43,7 +43,6 @@ setup
teardown
{
DROP TABLE t;
- DROP EXTENSION injection_points;
}
session s1
diff --git a/src/test/modules/injection_points/specs/inplace.spec b/src/test/modules/injection_points/specs/inplace.spec
index 86539a5bd2f..090166ebb0c 100644
--- a/src/test/modules/injection_points/specs/inplace.spec
+++ b/src/test/modules/injection_points/specs/inplace.spec
@@ -9,7 +9,7 @@
# Just to save on filesystem syscalls, use relkind=c for every other rel.
setup
{
- CREATE EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
CREATE SCHEMA vactest;
CREATE FUNCTION vactest.mkrels(text, int, int) RETURNS void
LANGUAGE plpgsql SET search_path = vactest AS $$
@@ -35,7 +35,6 @@ setup
teardown
{
DROP SCHEMA vactest CASCADE;
- DROP EXTENSION injection_points;
}
# Wait during inplace update, in a VACUUM of vactest.orig50.
diff --git a/src/test/modules/injection_points/specs/on_conflict_probe_window.spec b/src/test/modules/injection_points/specs/on_conflict_probe_window.spec
index 78dddbe8b92..443a30b217a 100644
--- a/src/test/modules/injection_points/specs/on_conflict_probe_window.spec
+++ b/src/test/modules/injection_points/specs/on_conflict_probe_window.spec
@@ -13,7 +13,7 @@
setup
{
- CREATE EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
CREATE TABLE probe_a (key int PRIMARY KEY, val int);
CREATE TABLE probe_b (key int PRIMARY KEY, val int);
INSERT INTO probe_a VALUES (1, 0);
@@ -24,7 +24,6 @@ setup
teardown
{
DROP TABLE probe_a, probe_b;
- DROP EXTENSION injection_points;
}
session s1
diff --git a/src/test/modules/injection_points/specs/reindex_concurrently_deferred.spec b/src/test/modules/injection_points/specs/reindex_concurrently_deferred.spec
index 4b95e1da2a7..748b8dee604 100644
--- a/src/test/modules/injection_points/specs/reindex_concurrently_deferred.spec
+++ b/src/test/modules/injection_points/specs/reindex_concurrently_deferred.spec
@@ -9,7 +9,7 @@
setup
{
- CREATE EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
CREATE TABLE reind_deferred (id int, val int,
CONSTRAINT uq_val UNIQUE(val) DEFERRABLE INITIALLY DEFERRED);
INSERT INTO reind_deferred VALUES (1, 1), (2, 2);
@@ -18,7 +18,6 @@ setup
teardown
{
DROP TABLE reind_deferred;
- DROP EXTENSION injection_points;
}
session s1
diff --git a/src/test/modules/injection_points/specs/repack.spec b/src/test/modules/injection_points/specs/repack.spec
index 7896d1456ad..500140b0fc0 100644
--- a/src/test/modules/injection_points/specs/repack.spec
+++ b/src/test/modules/injection_points/specs/repack.spec
@@ -1,7 +1,7 @@
# REPACK (CONCURRENTLY) ... USING INDEX ...;
setup
{
- CREATE EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
CREATE TABLE repack_test(i int PRIMARY KEY, j int,
k int GENERATED ALWAYS AS (j * 2) STORED);
@@ -16,7 +16,6 @@ setup
teardown
{
DROP TABLE repack_test;
- DROP EXTENSION injection_points;
DROP TABLE relfilenodes;
DROP TABLE data_s1;
diff --git a/src/test/modules/injection_points/specs/repack_commit_race.spec b/src/test/modules/injection_points/specs/repack_commit_race.spec
index 9926a45839e..05627fcda82 100644
--- a/src/test/modules/injection_points/specs/repack_commit_race.spec
+++ b/src/test/modules/injection_points/specs/repack_commit_race.spec
@@ -5,7 +5,7 @@
# for MVCC correctness.
setup
{
- CREATE EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
CREATE TABLE repack_race(i int PRIMARY KEY, j int);
INSERT INTO repack_race(i, j) VALUES (1, 1), (2, 2);
@@ -14,7 +14,6 @@ setup
teardown
{
DROP TABLE repack_race;
- DROP EXTENSION injection_points;
}
session s1
diff --git a/src/test/modules/injection_points/specs/repack_decode.spec b/src/test/modules/injection_points/specs/repack_decode.spec
index 04f0df83a0a..522ec41cbac 100644
--- a/src/test/modules/injection_points/specs/repack_decode.spec
+++ b/src/test/modules/injection_points/specs/repack_decode.spec
@@ -1,6 +1,6 @@
setup
{
- CREATE EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
BEGIN;
-- Generate a string of random characters that is not likely to be
@@ -22,7 +22,6 @@ setup
teardown
{
DROP TABLE repack_toast;
- DROP EXTENSION injection_points;
DROP FUNCTION gen_external();
SELECT pg_drop_replication_slot('s');
}
diff --git a/src/test/modules/injection_points/specs/repack_dropped.spec b/src/test/modules/injection_points/specs/repack_dropped.spec
index 099cf744202..784497b0f30 100644
--- a/src/test/modules/injection_points/specs/repack_dropped.spec
+++ b/src/test/modules/injection_points/specs/repack_dropped.spec
@@ -4,7 +4,7 @@
# they'll comfortably fit in a single page. The OLD tuple 1 is propagated
# through the concurrent update because of the trigger.
setup {
- CREATE EXTENSION injection_points;
+ 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;
diff --git a/src/test/modules/injection_points/specs/repack_missingval.spec b/src/test/modules/injection_points/specs/repack_missingval.spec
index 4b70d810afd..df7b1f8d839 100644
--- a/src/test/modules/injection_points/specs/repack_missingval.spec
+++ b/src/test/modules/injection_points/specs/repack_missingval.spec
@@ -29,7 +29,6 @@ teardown
{
DROP TABLE repack_missingval;
DROP FUNCTION repack_return_old();
- DROP EXTENSION injection_points;
}
session s1
diff --git a/src/test/modules/injection_points/specs/repack_temporal.spec b/src/test/modules/injection_points/specs/repack_temporal.spec
index c9fd3f61a84..f29b7e573c4 100644
--- a/src/test/modules/injection_points/specs/repack_temporal.spec
+++ b/src/test/modules/injection_points/specs/repack_temporal.spec
@@ -6,7 +6,7 @@
# target row.
setup
{
- CREATE EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
CREATE TABLE repack_temporal (
id int4range,
@@ -28,7 +28,6 @@ setup
teardown
{
DROP TABLE repack_temporal;
- DROP EXTENSION injection_points;
DROP TABLE relfilenodes;
}
diff --git a/src/test/modules/injection_points/specs/repack_temporal_multirange.spec b/src/test/modules/injection_points/specs/repack_temporal_multirange.spec
index 8150c0b4ba8..e1328409a9d 100644
--- a/src/test/modules/injection_points/specs/repack_temporal_multirange.spec
+++ b/src/test/modules/injection_points/specs/repack_temporal_multirange.spec
@@ -6,7 +6,7 @@
# can produce both as candidates and requires exact recheck.
setup
{
- CREATE EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
CREATE TABLE repack_temporal_multirange (
id int4multirange,
@@ -34,7 +34,6 @@ setup
teardown
{
DROP TABLE repack_temporal_multirange;
- DROP EXTENSION injection_points;
DROP TABLE relfilenodes;
}
diff --git a/src/test/modules/injection_points/specs/repack_toast.spec b/src/test/modules/injection_points/specs/repack_toast.spec
index bce4d85f4d9..187ba0616e3 100644
--- a/src/test/modules/injection_points/specs/repack_toast.spec
+++ b/src/test/modules/injection_points/specs/repack_toast.spec
@@ -67,7 +67,6 @@ setup
teardown
{
DROP TABLE repack_toast;
- DROP EXTENSION injection_points;
DROP FUNCTION gen_compressible(int);
DROP FUNCTION gen_compressible_external(int);
DROP FUNCTION gen_external();
diff --git a/src/test/modules/injection_points/specs/ri_fastpath_reindex.spec b/src/test/modules/injection_points/specs/ri_fastpath_reindex.spec
index bc1ec6e2819..01127c8cf7d 100644
--- a/src/test/modules/injection_points/specs/ri_fastpath_reindex.spec
+++ b/src/test/modules/injection_points/specs/ri_fastpath_reindex.spec
@@ -1,4 +1,3 @@
-# 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
# REINDEX CONCURRENTLY to repoint the constraint. Verify that the check
@@ -7,7 +6,7 @@
setup
{
- CREATE EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
CREATE TABLE ri_pk (id int PRIMARY KEY);
INSERT INTO ri_pk SELECT g FROM generate_series(1, 100) g;
CREATE TABLE ri_fk (id int PRIMARY KEY, pid int REFERENCES ri_pk(id));
@@ -19,7 +18,6 @@ setup
teardown
{
DROP TABLE ri_fk, ri_pk, ri_old_index;
- DROP EXTENSION injection_points;
}
# The rebuild, stopped just before it repoints the constraint.
diff --git a/src/test/modules/injection_points/specs/ri_fastpath_snapshot.spec b/src/test/modules/injection_points/specs/ri_fastpath_snapshot.spec
index a3cf6c30fb4..8381d441915 100644
--- a/src/test/modules/injection_points/specs/ri_fastpath_snapshot.spec
+++ b/src/test/modules/injection_points/specs/ri_fastpath_snapshot.spec
@@ -6,7 +6,7 @@
setup
{
- CREATE EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
CREATE ROLE regress_ri_snapshot;
CREATE TABLE ri_snapshot_pk (id int PRIMARY KEY);
CREATE TABLE ri_snapshot_fk (pid int);
@@ -20,7 +20,6 @@ teardown
{
DROP TABLE ri_snapshot_fk, ri_snapshot_pk;
DROP ROLE regress_ri_snapshot;
- DROP EXTENSION injection_points;
}
session s1
diff --git a/src/test/modules/injection_points/specs/syscache-update-pruned.spec b/src/test/modules/injection_points/specs/syscache-update-pruned.spec
index e3a4295bd12..367780eba93 100644
--- a/src/test/modules/injection_points/specs/syscache-update-pruned.spec
+++ b/src/test/modules/injection_points/specs/syscache-update-pruned.spec
@@ -25,7 +25,7 @@
# on filesystem syscalls, use relkind=c for every other rel.
setup
{
- CREATE EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
CREATE SCHEMA vactest;
-- Ensure a leader RELOID catcache entry. PARALLEL RESTRICTED since a
-- parallel worker running pg_relation_filenode() would lack that effect.
@@ -94,7 +94,6 @@ setup
teardown
{
DROP SCHEMA vactest CASCADE;
- DROP EXTENSION injection_points;
}
# Wait during GRANT. Disable debug_discard_caches, since we're here to
diff --git a/src/test/modules/injection_points/specs/wait_cleanup.spec b/src/test/modules/injection_points/specs/wait_cleanup.spec
index ed7d21c4de4..beefa4e4a71 100644
--- a/src/test/modules/injection_points/specs/wait_cleanup.spec
+++ b/src/test/modules/injection_points/specs/wait_cleanup.spec
@@ -5,11 +5,7 @@
setup
{
- CREATE EXTENSION injection_points;
-}
-teardown
-{
- DROP EXTENSION injection_points;
+ CREATE EXTENSION IF NOT EXISTS injection_points;
}
# The first waiter, that gets canceled or terminated. This does not
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
2026-09-30 06:27 REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 07:37 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 11:48 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Álvaro Herrera <alvherre@kurilemu.de>
2026-09-30 14:41 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Antonin Houska <ah@cybertec.at>
2026-09-30 15:38 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-10-03 11:35 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Alvaro Herrera <alvherre@kurilemu.de>
2026-10-05 03:51 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Fujii Masao <masao.fujii@gmail.com>
2026-10-05 11:35 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Alvaro Herrera <alvherre@kurilemu.de>
@ 2026-10-05 15:46 ` shihao zhong <zhong950419@gmail.com>
0 siblings, 0 replies; 10+ messages in thread
From: shihao zhong @ 2026-10-05 15:46 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@kurilemu.de>; +Cc: Fujii Masao <masao.fujii@gmail.com>; Radim Marek <radim@boringsql.com>; Antonin Houska <ah@cybertec.at>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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
Attachments:
[application/x-patch] v1-0002-Test-dropped-column-values-in-inserts-replayed-by.patch (3.1K, ../../CAGRkXqTxse0sTb43z3kFAHDrffjkamHkFGfoLEtwRkr_QfDGGg@mail.gmail.com/3-v1-0002-Test-dropped-column-values-in-inserts-replayed-by.patch)
download | inline diff:
From dabe9f8196eeacf708d1dcf227efd96bcf9cbce0 Mon Sep 17 00:00:00 2001
From: shihao zhong <zhong950419@gmail.com>
Date: Mon, 5 Oct 2026 00:18:57 -0400
Subject: [PATCH v1 2/2] Test dropped-column values in inserts replayed by
REPACK
Add a concurrent INSERT to repack_dropped. A BEFORE INSERT trigger
makes the new row a copy of an existing one, so it carries the value of
the dropped column.
Discussion: https://postgr.es/m/CAJgoLkK2UBzB1J9buCsSUbjf7bOqz-o_0=CeiTruU789BhBw-Q@mail.gmail.com
---
.../injection_points/expected/repack_dropped.out | 8 ++++++--
.../injection_points/specs/repack_dropped.spec | 16 +++++++++++++++-
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/src/test/modules/injection_points/expected/repack_dropped.out b/src/test/modules/injection_points/expected/repack_dropped.out
index 8aaab73aebb..4186ce4981c 100644
--- a/src/test/modules/injection_points/expected/repack_dropped.out
+++ b/src/test/modules/injection_points/expected/repack_dropped.out
@@ -1,6 +1,6 @@
Parsed test spec with 3 sessions
-starting permutation: s1_size s2_repack s3_updates s1_unlock s2_noop s1_size
+starting permutation: s1_size s2_repack s3_updates s3_insert s1_unlock s2_noop s1_size
injection_points_attach
-----------------------
@@ -23,6 +23,9 @@ step s2_repack:
step s3_updates:
UPDATE repack_dropped SET a = a || a;
+step s3_insert:
+ INSERT INTO repack_dropped (id, a) VALUES (3, 'three');
+
step s1_unlock:
SELECT injection_points_wakeup('repack-concurrently-before-lock');
@@ -43,5 +46,6 @@ id|fits_in_one_block
--+-----------------
1|t
2|t
-(2 rows)
+ 3|t
+(3 rows)
diff --git a/src/test/modules/injection_points/specs/repack_dropped.spec b/src/test/modules/injection_points/specs/repack_dropped.spec
index 099cf744202..57700968032 100644
--- a/src/test/modules/injection_points/specs/repack_dropped.spec
+++ b/src/test/modules/injection_points/specs/repack_dropped.spec
@@ -13,11 +13,18 @@ setup {
INSERT INTO repack_dropped (id, a, b) VALUES (2, 'two',
repeat(encode(sha256('2'), 'hex'), current_setting('block_size')::int / 32));
CREATE FUNCTION repack_dropped_f() RETURNS trigger LANGUAGE plpgsql AS
- $$ BEGIN
+ $$ DECLARE r repack_dropped;
+ BEGIN
+ IF TG_OP = 'INSERT' THEN
+ r := (SELECT t FROM repack_dropped t WHERE id = 1);
+ r.id := NEW.id;
+ RETURN r;
+ END IF;
IF NEW.id = 1 THEN return OLD; END IF;
RETURN NEW;
END $$;
CREATE TRIGGER repack_dropped_t BEFORE UPDATE ON repack_dropped FOR EACH ROW EXECUTE FUNCTION repack_dropped_f();
+ CREATE TRIGGER repack_dropped_ins_t BEFORE INSERT ON repack_dropped FOR EACH ROW EXECUTE FUNCTION repack_dropped_f();
ALTER TABLE repack_dropped DROP COLUMN b;
}
@@ -62,10 +69,17 @@ step s3_updates
UPDATE repack_dropped SET a = a || a;
}
+# The trigger makes tuple 3 a copy of tuple 1, dropped column included.
+step s3_insert
+{
+ INSERT INTO repack_dropped (id, a) VALUES (3, 'three');
+}
+
permutation
s1_size
s2_repack
s3_updates
+ s3_insert
s1_unlock
s2_noop
s1_size
--
2.37.1 (Apple Git-137.1)
[application/x-patch] v1-0001-Clear-out-dropped-column-values-from-inserts-repl.patch (3.5K, ../../CAGRkXqTxse0sTb43z3kFAHDrffjkamHkFGfoLEtwRkr_QfDGGg@mail.gmail.com/4-v1-0001-Clear-out-dropped-column-values-from-inserts-repl.patch)
download | inline diff:
From 09555e6c8b72da4a6f133f683286895c611cc4e5 Mon Sep 17 00:00:00 2001
From: shihao zhong <zhong950419@gmail.com>
Date: Mon, 5 Oct 2026 00:18:57 -0400
Subject: [PATCH v1 1/2] Clear out dropped-column values from inserts replayed
by REPACK
e5d25959cf8 cleared values of dropped columns only when replaying an
UPDATE. A concurrent INSERT can carry one too, for example when a
trigger returns a copy of an existing row. REPACK (CONCURRENTLY) then
kept it, or failed with "row is too big" if the new heap has no TOAST
table. Clear them in restore_tuple() instead.
Discussion: https://postgr.es/m/CAJgoLkK2UBzB1J9buCsSUbjf7bOqz-o_0=CeiTruU789BhBw-Q@mail.gmail.com
---
src/backend/commands/repack.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 899005609c2..7870ac6607b 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -2819,7 +2819,7 @@ apply_concurrent_changes(BufFile *file, ChangeContext *chgcxt)
/*
* Adjust spilled_tuple so that it can be used as the new tuple in
* the update that we're about to replay. This fixes TOAST
- * pointers as well as remove useless values from dropped columns.
+ * pointers.
*/
prepare_concurrent_update(spilled_tuple, ondisk_tuple);
@@ -2946,6 +2946,7 @@ apply_concurrent_delete(Relation rel, TupleTableSlot *slot)
static void
restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot)
{
+ TupleDesc desc = slot->tts_tupleDescriptor;
uint32 t_len;
HeapTuple tup;
int natt_ext;
@@ -2965,6 +2966,17 @@ restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot)
*/
ExecForceStoreHeapTuple(tup, slot, false);
+ /*
+ * Dropped columns can still have values in the tuple. Mark them as null:
+ * they'd waste space, and the new heap might have no TOAST table to store
+ * them in.
+ */
+ for (int i = 0; i < desc->natts; i++)
+ {
+ if (TupleDescCompactAttr(desc, i)->attisdropped)
+ slot->tts_isnull[i] = true;
+ }
+
/*
* Next, read any attributes we stored separately into the tts_values
* array elements expecting them, if any. This matches
@@ -2973,8 +2985,6 @@ restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot)
BufFileReadExact(file, &natt_ext, sizeof(natt_ext));
if (natt_ext > 0)
{
- TupleDesc desc = slot->tts_tupleDescriptor;
-
for (int i = 0; i < desc->natts; i++)
{
CompactAttribute *attr = TupleDescCompactAttr(desc, i);
@@ -3020,10 +3030,6 @@ restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot)
* - Any EXTERNAL_ONDISK toast pointers so that it points to the corresponding
* toast value in 'src' (the transient table) instead. The TOAST storage for
* 'dest' is going to be dropped, so these values cannot be used any longer.
- *
- * We also apply the following optimization:
- * - If any columns are dropped but the slot still contains values, mark them
- * as null to avoid uselessly wasting space in the new relation.
*/
static void
prepare_concurrent_update(TupleTableSlot *dest, TupleTableSlot *src)
@@ -3036,14 +3042,7 @@ prepare_concurrent_update(TupleTableSlot *dest, TupleTableSlot *src)
varlena *varlena_dst;
if (attr->attisdropped)
- {
- if (!slot_attisnull(dest, i + 1))
- {
- slot_getsomeattrs(dest, i + 1);
- dest->tts_isnull[i] = true;
- }
continue;
- }
if (attr->attlen != -1)
continue;
if (slot_attisnull(dest, i + 1))
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: REPACK (CONCURRENTLY) might keep dropped-column data
2026-09-30 06:27 REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 07:37 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Radim Marek <radim@boringsql.com>
2026-09-30 11:48 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Álvaro Herrera <alvherre@kurilemu.de>
2026-09-30 14:41 ` Re: REPACK (CONCURRENTLY) might keep dropped-column data Antonin Houska <ah@cybertec.at>
@ 2026-10-03 13:48 ` Alvaro Herrera <alvherre@kurilemu.de>
1 sibling, 0 replies; 10+ 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] 10+ messages in thread
end of thread, other threads:[~2026-10-05 15:46 UTC | newest]
Thread overview: 10+ 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-05 03:51 ` Fujii Masao <masao.fujii@gmail.com>
2026-10-05 11:35 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-10-05 15:46 ` shihao zhong <zhong950419@gmail.com>
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