DKIM2-Signature: i=1; m=1; t=1791027316; d=kurilemu.de;
	mf=PGFsdmhlcnJlQGt1cmlsZW11LmRlPg==;
	rt=PHBnc3FsLWhhY2tlcnNAbGlzdHMucG9zdGdyZXNxbC5vcmc+;
	s=fm3:rsa-sha256:2q/+nSV4wJRA99KqBzOHoy7kvvpWhWGZ6swPBDhhn0lyCqn
	MCDVxNu3/F4LapM0vkw0h1MmSsV4fCUvHwOmdPgThtN07mIr6bMRJY+n10EeF98N
	ZYsmjtgCNmTHzpfryOlWKtjlf9wZW19Gx49xVswW/G2dIxmqTGh2uS0iMWjlgjGK
	pTE4iHMBM7Q7IkntJv8peD46+XixBLD2HPyKGKe6hPuUth+7X6WNsXBoAIDxy+eX
	/zwysn717dBmmwBmX98cjiqqxiVD6tUEW8TshnisxoqI0u5MKFp9Kkm5mjo8OvFT
	H4FXlXUEebdSTKUvf/z0WTq7BvKZcEdUA+Sqibw==;
Message-Instance: m=1;
 h=sha256:cv4U52kFdx0LDx+UVp+FWIiTMTepBRDKqTZFeM7TK04=:ALMY1KWEu7r+C7pJy7s2BT1MKncxXAPsz33mDiwPkIw=;
Feedback-ID: ie3de48e3:Fastmail
Date: Sat, 3 Oct 2026 13:35:14 +0200
From: Alvaro Herrera <alvherre@kurilemu.de>
To: Radim Marek <radim@boringsql.com>
Cc: Antonin Houska <ah@cybertec.at>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: REPACK (CONCURRENTLY) might keep dropped-column data
Message-ID: <asDkn09-y9N1_FJQ@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAJgoLkLWCw6+v6zL5bb3Tjvz=+EZ169mFrmpOh7xd45D2tSxrA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/asDkn09-y9N1_FJQ%40alvherre.pgsql>
Precedence: bulk

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"