Assertion failure in GetSubscriptionRelations() with concurrent DROP TABLE
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253526psql -h localhost -U postgresBuilt from patchset v14 (message #14), August 26, 2026 at 06:08 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253526_14 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253526_14 && git checkout t253526_14Patchset v14 (message #14) is on t253526_14
Hi,
I found an issue in GetSubscriptionRelations() when a REFRESH
PUBLICATION is executed while a subscribed relation is dropped
concurrently.
GetSubscriptionRelations() scans pg_subscription_rel using the catalog
snapshot taken when the scan starts, and then looks up the relation's
relkind through the syscache. If the relation is dropped and the drop
commits, the pg_subscription_rel row can still be visible while the
corresponding pg_class row is already gone. In this case,
get_rel_relkind() returns '\0'.
This can be reproduced with the following steps:
I have reproduced it through debugger:
step 1: Prepare logical replication setup for table tbl1,
step 2: session 1: ALTER SUBSCRIPTION sub1 REFRESH PUBLICATION; -->
Hold this statement through debugger at GetSubscriptionRelations
function just before get_rel_relkind function call
step 3: session 2: DROP TABLE tbl1; --> Drop the table concurrently
while the session 1 is stopped at GetSubscriptionRelations
step 4: continue the execution to see the Assertion failure.
get_rel_relkind will return '\0' which causes the assertion failure at:
....
relkind = get_rel_relkind(subrel->srrelid);
Assert(relkind == RELKIND_SEQUENCE || relkind == RELKIND_RELATION ||
relkind == RELKIND_PARTITIONED_TABLE);
....
This caused the following assertion to fail in an assert-enabled build:
TRAP: failed Assert("relkind == RELKIND_SEQUENCE || relkind ==
RELKIND_RELATION || relkind == RELKIND_PARTITIONED_TABLE"), File:
"pg_subscription.c", Line: 699, PID: 32422
0 postgres 0x0000000102a47728
ExceptionalCondition + 216
1 postgres 0x00000001022ab850
GetSubscriptionRelations + 484
2 postgres 0x00000001023e52a8
AlterSubscription_refresh + 620
Attached is 0001-Test-to-reproduce-assertion-failure-in.patch, which
adds an injection-point test to reproduce the issue.
The fix is to skip the relation when get_rel_relkind() returns '\0',
since the relation has already been dropped and there is nothing left
to synchronise. The attached
v1-0001-Skip-relations-dropped-concurrently-in-GetSubscri.patch patch
has the fix for the same. This also needs to be fixed in the v19
branch, where get_rel_relkind() was introduced by commit
f0b3573c3aac6c0ea4cbc278f98178516579d370.
Regards,
Vignesh
Attachments:
t253526_1v1-0001-Skip-relations-dropped-concurrently-in-GetSubscri.patchapplication/octet-stream; name=v1-0001-Skip-relations-dropped-concurrently-in-GetSubscri.patchDownload+6-2
0001-Test-to-reproduce-assertion-failure-in.patchapplication/octet-stream; name=0001-Test-to-reproduce-assertion-failure-in.patchDownload+159-1
Hi,
On Mon, Aug 24, 2026 at 5:01 AM vignesh C <vignesh21@gmail.com> wrote:
I found an issue in GetSubscriptionRelations() when a REFRESH
PUBLICATION is executed while a subscribed relation is dropped
concurrently.
Thanks for reporting and sending the patch.
GetSubscriptionRelations() scans pg_subscription_rel using the catalog
snapshot taken when the scan starts, and then looks up the relation's
relkind through the syscache. If the relation is dropped and the drop
commits, the pg_subscription_rel row can still be visible while the
corresponding pg_class row is already gone. In this case,
get_rel_relkind() returns '\0'.This can be reproduced with the following steps:
I have reproduced it through debugger:get_rel_relkind will return '\0' which causes the assertion failure at:
....
relkind = get_rel_relkind(subrel->srrelid);Assert(relkind == RELKIND_SEQUENCE || relkind == RELKIND_RELATION ||
relkind == RELKIND_PARTITIONED_TABLE);
How easy is it to hit this with direct SQL without a debugger or
injection point? In production builds (no assertions), what happens
when the relation is concurrently dropped? Does the logical
replication worker error or crash, and if it exits, does it recover in
the next cycle since the relation won't be in the list anymore?
If it's not easily reproducible with direct SQL, I don't think we need
to fix this at all, because none of the other places (for example,
getOwnedSequences_internal() or GetPubPartitionOptionRelations())
handle a null relkind either. Even if we were to fix, I would just
remove the assertion and continue in the loop if the relation is not
what we expect. Would that work?
--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
On Mon, Aug 24, 2026 at 11:35 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
If it's not easily reproducible with direct SQL, I don't think we need
to fix this at all, because none of the other places (for example,
getOwnedSequences_internal() or GetPubPartitionOptionRelations())
handle a null relkind either.
But they will work (as they are if checks) and as far as I can see
they will do the right thing which means they will tolerate concurrent
drops but if not I feel it is better to fix those as well.
Even if we were to fix, I would just
remove the assertion and continue in the loop if the relation is not
what we expect. Would that work?
As we are scanning pg_susbcription_rel unexpected relkind here (not
'\0', something else entirely) would probably indicate a real bug
worth surfacing, not silently swallowing. So, I suggest we can remove
assert after Vignesh's fix and have an elog(ERROR, "unexpected
relkind: %d", (int) relkind);. We use such an elog at other places as
well.
--
With Regards,
Amit Kapila.
Hi,
On Mon, Aug 24, 2026 at 9:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Aug 24, 2026 at 11:35 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:If it's not easily reproducible with direct SQL, I don't think we need
to fix this at all, because none of the other places (for example,
getOwnedSequences_internal() or GetPubPartitionOptionRelations())
handle a null relkind either.But they will work (as they are if checks) and as far as I can see
they will do the right thing which means they will tolerate concurrent
drops but if not I feel it is better to fix those as well.
Yes, it's worth exploring all of these in the logical replication code
(maybe separately though).
Even if we were to fix, I would just
remove the assertion and continue in the loop if the relation is not
what we expect. Would that work?As we are scanning pg_susbcription_rel unexpected relkind here (not
'\0', something else entirely) would probably indicate a real bug
worth surfacing, not silently swallowing. So, I suggest we can remove
assert after Vignesh's fix and have an elog(ERROR, "unexpected
relkind: %d", (int) relkind);. We use such an elog at other places as
well.
Works for me, and I think both can go in one patch. After the relkind
fix, replacing the assertion with an error means that in the rare
scenario where the OID of the dropped table gets reused to create a
relation that's not expected, we catch it. I'm all in favor of
tightening this anyway.
On backpatching this fix, I think HEAD and PG19 (still in beta) are
enough, because it's not easily reproducible with direct SQL. For the
same reason, I'm fine with not having an injection point and a TAP
test for this.
--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
On Tue, 25 Aug 2026 at 11:03, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
Yes, it's worth exploring all of these in the logical replication code
(maybe separately though).
I Agree, if there is any issue, it can be handled separately.
Even if we were to fix, I would just
remove the assertion and continue in the loop if the relation is not
what we expect. Would that work?As we are scanning pg_susbcription_rel unexpected relkind here (not
'\0', something else entirely) would probably indicate a real bug
worth surfacing, not silently swallowing. So, I suggest we can remove
assert after Vignesh's fix and have an elog(ERROR, "unexpected
relkind: %d", (int) relkind);. We use such an elog at other places as
well.Works for me, and I think both can go in one patch.
Updated
After the relkind
fix, replacing the assertion with an error means that in the rare
scenario where the OID of the dropped table gets reused to create a
relation that's not expected, we catch it. I'm all in favor of
tightening this anyway.
The other ways this could happen are: (a) if logical replication is
extended in the future to support additional relation types, or (b) if
the system catalog is corrupted, for example by directly updating
relkind in pg_class using an UPDATE statement.
On backpatching this fix, I think HEAD and PG19 (still in beta) are
enough, because it's not easily reproducible with direct SQL. For the
same reason, I'm fine with not having an injection point and a TAP
test for this.
Yes, the injection-point test does not need to be committed. I shared
it only to make the issue easier to reproduce.
The attached v2 version patch has the changes for the same.
Regards,
Vignesh
Dear Vignesh,
Thanks for updating the patch. I confirmed it could fix the issue.
```
+ else
+ elog(ERROR, "unexpected relkind: %d", (int) relkind);
```I feel \"%c\" might be better than %d, like what ProcessUtilitySlow() does,
because the relkind is defined as char.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
On Tue, Aug 25, 2026 at 5:39 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
Thanks for updating the patch. I confirmed it could fix the issue.
``` + else + elog(ERROR, "unexpected relkind: %d", (int) relkind); ```I feel \"%c\" might be better than %d, like what ProcessUtilitySlow() does,
because the relkind is defined as char.
Agreed, though %d is used for the same purpose at one place in code
but other uses %c for relkind. How about a slightly more informative
message like: elog(ERROR, "unexpected relkind \"%c\" for relation %u
in subscription %u", relkind, subrel->srrelid, subid)?
--
With Regards,
Amit Kapila.
Does this one deserve a mention on the open items wiki [0]https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items?
[0]: https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items
--
nathan
Hi,
On Tue, Aug 25, 2026 at 12:53 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
Does this one deserve a mention on the open items wiki [0]?
[0] https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items
I don't think so TBH. Given that it's not easily reproducible with
direct SQL and we seem to have agreement on the fix. However, I will
wait for Amit/Vignesh or others here.
--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
Hello,
Both changes in the current patch look good to me.
At Tue, 25 Aug 2026 19:11:27 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
I feel \"%c\" might be better than %d, like what ProcessUtilitySlow() does,
because the relkind is defined as char.Agreed, though %d is used for the same purpose at one place in code
but other uses %c for relkind. How about a slightly more informative
message like: elog(ERROR, "unexpected relkind \"%c\" for relation %u
in subscription %u", relkind, subrel->srrelid, subid)?
I may be being overly cautious, but I'm a little hesitant to use %c
here. For example, if the value happened to be the first byte of a
UTF-8 sequence, it could result in an invalidly encoded log line.
Since this error is unlikely to be seen in practice and readability is
not particularly important here, I think %d is safer. It might even be
better to change the existing uses of %c for relkind to %d for the
same reason.
Regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Wed, 26 Aug 2026 at 07:32, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
Hello,
Both changes in the current patch look good to me.
At Tue, 25 Aug 2026 19:11:27 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
I feel \"%c\" might be better than %d, like what ProcessUtilitySlow() does,
because the relkind is defined as char.Agreed, though %d is used for the same purpose at one place in code
but other uses %c for relkind. How about a slightly more informative
message like: elog(ERROR, "unexpected relkind \"%c\" for relation %u
in subscription %u", relkind, subrel->srrelid, subid)?I may be being overly cautious, but I'm a little hesitant to use %c
here. For example, if the value happened to be the first byte of a
UTF-8 sequence, it could result in an invalidly encoded log line.Since this error is unlikely to be seen in practice and readability is
not particularly important here, I think %d is safer. It might even be
better to change the existing uses of %c for relkind to %d for the
same reason.
I checked how relkind is formatted elsewhere in the codebase.
%c is used more commonly like in the following places:
heapam.c — "missing lock for relation \"%s\" (OID %u, relkind %c) @ TID (%u,%u)"
utility.c — "unexpected relkind \"%c\" on partition \"%s\""
postgres_fdw.c — "remote table \"%s.%s\" is of relkind \"%c\" which
cannot have statistics"
pg_dump.c — "cannot dump statistics for relation kind \"%c\""
pg_class.c — "unrecognized relkind: '%c'"
%d is used in only one place:
tablecmds.c — "unexpected relkind: %d"
So %c is clearly the more common convention for relkind. I think we
should follow the existing convention and use %c in this patch. If we
decide that %d is preferable, it would be better to change all of the
%c uses in a separate patch.
Thoughts?
Regards,
Vignesh
On Wed, Aug 26, 2026 at 7:32 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
Both changes in the current patch look good to me.
At Tue, 25 Aug 2026 19:11:27 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
I feel \"%c\" might be better than %d, like what ProcessUtilitySlow() does,
because the relkind is defined as char.Agreed, though %d is used for the same purpose at one place in code
but other uses %c for relkind. How about a slightly more informative
message like: elog(ERROR, "unexpected relkind \"%c\" for relation %u
in subscription %u", relkind, subrel->srrelid, subid)?I may be being overly cautious, but I'm a little hesitant to use %c
here. For example, if the value happened to be the first byte of a
UTF-8 sequence, it could result in an invalidly encoded log line.
I think the UTF-8 concern would apply only if relkind contained a
value outside the currently defined RELKIND_* values. All the valid
relkind values defined in pg_class.h are ASCII characters (r, i, S,
t,etc.), so %c is safe for the values that relkind is expected to
contain.
That said, I agree that %d would be safer if we want to make the error
handling robust against an arbitrary/invalid byte say due to catalog
corruption. As Vignesh pointed out, though, %c is currently the common
convention for printing relkind, whereas %d is used only in one place.
So perhaps we should keep %c in this patch and, if we want to adopt %d
for unexpected relkind values, let's handle the existing %c usages
consistently in a separate patch.
Thoughts?
--
With Regards,
Amit Kapila.
Hi,
On Tue, Aug 25, 2026 at 9:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
That said, I agree that %d would be safer if we want to make the error
handling robust against an arbitrary/invalid byte say due to catalog
corruption. As Vignesh pointed out, though, %c is currently the common
convention for printing relkind, whereas %d is used only in one place.So perhaps we should keep %c in this patch and, if we want to adopt %d
for unexpected relkind values, let's handle the existing %c usages
consistently in a separate patch.Thoughts?
I looked at the most common and widely used path, which is
errdetail_relkind_not_supported(). It uses %c (some of these might be
user-visible errors). I prefer to go with that, so +1 to use %c in
this patch and discuss the use of %d separately if that's a concern.
--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
On Tue, 25 Aug 2026 at 19:11, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 25, 2026 at 5:39 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:Thanks for updating the patch. I confirmed it could fix the issue.
``` + else + elog(ERROR, "unexpected relkind: %d", (int) relkind); ```I feel \"%c\" might be better than %d, like what ProcessUtilitySlow() does,
because the relkind is defined as char.Agreed, though %d is used for the same purpose at one place in code
but other uses %c for relkind. How about a slightly more informative
message like: elog(ERROR, "unexpected relkind \"%c\" for relation %u
in subscription %u", relkind, subrel->srrelid, subid)?
Here is an updated version with the message updated accordingly.
Regards,
Vignesh
Attachments:
t253526_14v3-0001-Skip-relations-dropped-concurrently-in-GetSubscri.patchapplication/octet-stream; name=v3-0001-Skip-relations-dropped-concurrently-in-GetSubscri.patchDownload+22-10
Hello,
At Tue, 25 Aug 2026 22:19:30 -0700, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
Hi,
On Tue, Aug 25, 2026 at 9:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
That said, I agree that %d would be safer if we want to make the error
handling robust against an arbitrary/invalid byte say due to catalog
corruption. As Vignesh pointed out, though, %c is currently the common
convention for printing relkind, whereas %d is used only in one place.So perhaps we should keep %c in this patch and, if we want to adopt %d
for unexpected relkind values, let's handle the existing %c usages
consistently in a separate patch.Thoughts?
I looked at the most common and widely used path, which is
errdetail_relkind_not_supported(). It uses %c (some of these might be
user-visible errors). I prefer to go with that, so +1 to use %c in
this patch and discuss the use of %d separately if that's a concern.
If this case has been considered and the choice of %c is intentional,
I have no objection to it.
Regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Wed, Aug 26, 2026 at 3:55 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
Hi,
On Tue, Aug 25, 2026 at 12:53 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:Does this one deserve a mention on the open items wiki [0]?
[0] https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items
I don't think so TBH. Given that it's not easily reproducible with
direct SQL and we seem to have agreement on the fix. However, I will
wait for Amit/Vignesh or others here.
I have pushed the fix. The reason to not add it to open items was that
the fix was simple and we have an agreement. Thanks for the review.
--
With Regards,
Amit Kapila.