MIME-Version: 1.0
References: <39977E94-40DD-4176-A191-CE3D120968B2@yandex-team.ru>
In-Reply-To: <39977E94-40DD-4176-A191-CE3D120968B2@yandex-team.ru>
From: surya poondla <suryapoondla4@gmail.com>
Date: Wed, 24 Jun 2026 16:56:10 -0700
Message-ID: 
 <CAOVWO5r3Zv-3ThZpXdqa66uMCLP__gj6BaiiFRPXJ1t8t6bzoA@mail.gmail.com>
Subject: Re: Two bugs around ALTER TYPE
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="00000000000059f97d0655089f9e"
Archived-At: 
 <https://www.postgresql.org/message-id/CAOVWO5r3Zv-3ThZpXdqa66uMCLP__gj6BaiiFRPXJ1t8t6bzoA%40mail.gmail.com>
Precedence: bulk

--00000000000059f97d0655089f9e
Content-Type: text/plain; charset="UTF-8"

Hi Andrey,

Thanks for the patch series. I was able to reproduce both bugs on master at
HEAD and verified each fix end-to-end:

A couple of points:
Bug 1: "Number of returned columns (2) does not match expected column count
(3) on the second call.
With 0002 patch, it returns the new 3-column shape as expected.

Bug 2 (with only 0004 patch): Session A returns (1,2), DDL commits after,
next call returns (1,2,99). Mechanism works as designed.


I also confirmed why glob->relationOids is the right channel rather than
glob->invalItems via record_plan_type_dependency(): ALTER TYPE ADD
ATTRIBUTE
on a composite goes through ALTER TABLE on typrelid and modifies
pg_class/pg_attribute but not the composite's pg_type row, so no TYPEOID
syscache invalidation fires
(CacheInvalidateHeapTuple in AlterDomainAddConstraint).
relcache-on-typrelid is the only channel that actually delivers, which is
what the patch hooks into.

A few comments:
1. 0003 patch alone leaves a permanently-failing TAP test (its expected
1|2|99 is only producible by 0004's lock forcing the DDL to
wait). Maybe equash 0003 patch into 0004 patch.

2. alter_table.sql comment says the fix records typrelid in "the plan's
invalItems"; it actually goes into glob->relationOids
different list, different callback (PlanCacheRelCallback vs
PlanCacheObjectCallback).

3. TAP test in 0004 captures $ddl_pid via pg_backend_pid() (lines 68-69)
but never uses it.
Either we can drop, or use it to assert the DDL session is genuinely
waiting on the typrelid in pg_locks
(granted=false on AccessExclusiveLock). The latter would directly verify
the lock instead of just observing the symptom.


Regards,
Surya Poondla

--00000000000059f97d0655089f9e
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><br>Hi Andrey,<br><br>Thanks for the patch series. I was a=
ble to reproduce both bugs on master at HEAD and verified each fix end-to-e=
nd:<br><br>A couple of points:<br>Bug 1: &quot;Number of returned columns (=
2) does not match expected column count (3) on the second call. <br>With 00=
02 patch, it returns the new 3-column shape as expected.<br><br>Bug 2 (with=
 only 0004 patch): Session A returns (1,2), DDL commits after, next call re=
turns (1,2,99). Mechanism works as designed.<br><br>=C2=A0=C2=A0<br>I also =
confirmed why glob-&gt;relationOids is the right channel rather than glob-&=
gt;invalItems via record_plan_type_dependency(): ALTER TYPE ADD ATTRIBUTE <=
br>on a composite goes through ALTER TABLE on typrelid and modifies pg_clas=
s/pg_attribute but not the composite&#39;s pg_type row, so no TYPEOID sysca=
che invalidation fires<br>(CacheInvalidateHeapTuple in AlterDomainAddConstr=
aint). relcache-on-typrelid is the only channel that actually delivers, whi=
ch is what the patch hooks into.<br><br>A few comments:<br>1. 0003 patch al=
one leaves a permanently-failing TAP test (its expected 1|2|99 is only prod=
ucible by 0004&#39;s lock forcing the DDL to<br>wait). Maybe equash 0003 pa=
tch into 0004 patch.<br><br>2. alter_table.sql comment says the fix records=
 typrelid in &quot;the plan&#39;s invalItems&quot;; it actually goes into g=
lob-&gt;relationOids<br>different list, different callback (PlanCacheRelCal=
lback vs PlanCacheObjectCallback).=C2=A0<br><br>3. TAP test in 0004 capture=
s $ddl_pid via pg_backend_pid() (lines 68-69) but never uses it. <br>Either=
 we can drop, or use it to assert the DDL session is genuinely waiting on t=
he typrelid in pg_locks<br>(granted=3Dfalse on AccessExclusiveLock). The la=
tter would directly verify the lock instead of just observing the symptom.<=
br><br><br>Regards,<br>Surya Poondla</div>

--00000000000059f97d0655089f9e--