Re: REPACK (CONCURRENTLY) might keep dropped-column data

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"