On Thu, Jul 31, 2025 at 2:37 PM vignesh C wrote:
>
> How about we change the below:
> +#ifdef USE_ASSERT_CHECKING
> + LOCKTAG tag;
> +#endif
> +
> + Assert(CheckRelationOidLockedByMe(SubscriptionRelRelationId,
> +
>RowExclusiveLock, true));
> +
> +
On Thu, 31 Jul 2025 at 08:23, Ajin Cherian wrote:
>
> On Wed, Jul 30, 2025 at 10:33 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Dear Ajin,
> >
> > > Attaching the updated patches with the changes you requested. I've
> > > also added the unchanged patches for PG_18 and HEAD (PG_18_HEAD-v6*),
> > >
Dear Ajin,
Thanks for updates. I confirmed that reported issues could be fixed by your
patch.
I have no comments anymore.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
On Wed, Jul 30, 2025 at 10:33 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Ajin,
>
> > Attaching the updated patches with the changes you requested. I've
> > also added the unchanged patches for PG_18 and HEAD (PG_18_HEAD-v6*),
> > so that everything is together in one mail.
>
> Thanks for update, b
Dear Ajin,
> Attaching the updated patches with the changes you requested. I've
> also added the unchanged patches for PG_18 and HEAD (PG_18_HEAD-v6*),
> so that everything is together in one mail.
Thanks for update, but the patch for PG18/HEAD seemed not to have Assert().
Can you modify like oth
On Tue, Jul 29, 2025 at 10:45 PM Hayato Kuroda (Fujitsu)
wrote:
>
> > > How do you feel the .diff file can be applied atop PG17 patch? It is
> > > mainly
> > > same as v4 patch but has some assertion.
>
> Sorry for my interrupted message. I noticed only I attached old version patch.
> PSA the cor
On Tue, 29 Jul 2025 at 14:46, Ajin Cherian wrote:
>
> On Tue, Jul 29, 2025 at 1:13 PM Amit Kapila wrote:
> >
> > Yes, that makes sense to me. For HEAD and PG18, we can still add a new
> > argument to the API. For other bank branches, it is better to use a
> > new Ex function as suggested by Kurod
> > How do you feel the .diff file can be applied atop PG17 patch? It is mainly
> > same as v4 patch but has some assertion.
Sorry for my interrupted message. I noticed only I attached old version patch.
PSA the correct version.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
kuroda.diffs
Descript
> How do you feel the .diff file can be applied atop PG17 patch? It is mainly
> same as v4 patch but has some assertion.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Dear Ajin,
> I have added "Ex functions" for back branches (PG_17 and earlier) ,
> which also have Asserts for making sure locks are taken. For PG_18 and
> HEAD, I've used the extra parameter already_locked.
> PG_14_15-v6-0001-Fix-a-deadlock-during-ALTER-SUBSCRIPTION.-DROP-PU.p
> atch
> is for bot
On Tue, Jul 29, 2025 at 4:26 PM vignesh C wrote:
>
> On Tue, 29 Jul 2025 at 14:46, Ajin Cherian wrote:
> >
> > On Tue, Jul 29, 2025 at 1:13 PM Amit Kapila wrote:
> > >
> > > Yes, that makes sense to me. For HEAD and PG18, we can still add a new
> > > argument to the API. For other bank branches,
On Tue, 29 Jul 2025 at 14:46, Ajin Cherian wrote:
>
> On Tue, Jul 29, 2025 at 1:13 PM Amit Kapila wrote:
> >
> > Yes, that makes sense to me. For HEAD and PG18, we can still add a new
> > argument to the API. For other bank branches, it is better to use a
> > new Ex function as suggested by Kurod
On Tue, Jul 29, 2025 at 1:13 PM Amit Kapila wrote:
>
> Yes, that makes sense to me. For HEAD and PG18, we can still add a new
> argument to the API. For other bank branches, it is better to use a
> new Ex function as suggested by Kuroda-San.
>
Here are the updated patches.
I have added "Ex funct
On Tue, Jul 29, 2025 at 7:23 AM Hayato Kuroda (Fujitsu)
wrote:
>
> > Thanks for your review Kuroda-san, I have changed the logic to not use
> > already_locked and instead check if the locks are taken inside
> > UpdateSubscriptionRelState itself. I've tested this and this works
> > fine. If this lo
Dear Ajin,
> Thanks for your review Kuroda-san, I have changed the logic to not use
> already_locked and instead check if the locks are taken inside
> UpdateSubscriptionRelState itself. I've tested this and this works
> fine. If this logic is acceptable to the reviewers I can update the
> other pa
On Fri, Jul 25, 2025 at 6:01 PM Hayato Kuroda (Fujitsu)
wrote:
> Also, I'm now bit confusing for which branches are really affected. Can you
> create all patches for branches, attach at the same e-mail and add some
> summary
> what you want to fix?
> E.g., you provided a patch for HEAD/PG15/PG14,
On Fri, Jul 25, 2025 at 6:01 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Ajin,
>
> Thanks for patches! While checking, I recalled that the backpatch policy [1].
> We must not modify definitions of opened functions but this does. Can you
> define
> another function like UpdateSubscriptionRelStateEx
On Thu, 24 Jul 2025 at 17:45, Ajin Cherian wrote:
>
> On Wed, Jul 23, 2025 at 8:01 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > > Dear Ajin,
> > >
> > > Thanks for the patch. Firstly let me confirm my understanding. While
> > > altering the
> > > subscription, locks are acquired with below orderi
Dear Ajin,
Thanks for patches! While checking, I recalled that the backpatch policy [1].
We must not modify definitions of opened functions but this does. Can you define
another function like UpdateSubscriptionRelStateEx or something on the back
branches?
Another comment:
```
@@ -328,9 +328,13 @@
On Wed, Jul 23, 2025 at 8:01 PM Hayato Kuroda (Fujitsu)
wrote:
>
> > Dear Ajin,
> >
> > Thanks for the patch. Firstly let me confirm my understanding. While
> > altering the
> > subscription, locks are acquired with below ordering:
> >
>
> I forgot to confirm one point. For which branch should be
On Wed, Jul 23, 2025 at 4:32 PM Amit Kapila wrote:
>
> On Mon, Jul 21, 2025 at 6:59 PM Ajin Cherian wrote:
> >
> > Yes, this is correct. I have also verified this in my testing that
> > when there is a second subscription, a deadlock can still happen with
> > my previous patch. I have now modifie
> Dear Ajin,
>
> Thanks for the patch. Firstly let me confirm my understanding. While altering
> the
> subscription, locks are acquired with below ordering:
>
I forgot to confirm one point. For which branch should be backpatch? Initially
it was reported only on PG15 [1], but I found 021_alter_su
On Wed, Jul 23, 2025 at 2:42 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Ajin,
>
> Thanks for the patch. Firstly let me confirm my understanding. While altering
> the
> subscription, locks are acquired with below ordering:
>
> Order target level
> 1
Dear Ajin,
Thanks for the patch. Firstly let me confirm my understanding. While altering
the
subscription, locks are acquired with below ordering:
Order target level
1 pg_subscription row exclusive
2 p
On Mon, Jul 21, 2025 at 6:59 PM Ajin Cherian wrote:
>
> Yes, this is correct. I have also verified this in my testing that
> when there is a second subscription, a deadlock can still happen with
> my previous patch. I have now modified the patch in tablesync worker
> to take locks on both Subscrip
> locked. I've made a new version of the patch on PG_15.
I've made a similar fix on HEAD just so that the code is now
consistent. I don't think the similar problem (deadlock between two
different subscriptions trying to drop tracking origin) occurs on
HEAD with my previous patch, as the way origi
On Thu, Jul 17, 2025 at 4:21 PM Amit Kapila wrote:
>
> It seems the patch assumes that the above lock is sufficient because
> AlterSubscription will take an AcessExclusiveLock on the same
> subscription. So, with this proposal, if both of those become
> serialized then the other locking order may
On Wed, Jul 16, 2025 at 8:38 AM Ajin Cherian wrote:
>
> Thanks for the test and confirming the fix. Fixed the comments.
>
* origin. So passing missing_ok = true.
+ *
+ * Also lock SubscriptionRelationId with AccessShareLock to
+ * prevent any deadlocks with the us
On Tue, Jul 15, 2025 at 2:24 PM vignesh C wrote:
>
> Couple of comments:
> 1) This change is not required:
> #include "utils/snapmgr.h"
> #include "utils/syscache.h"
> #include "utils/usercontext.h"
> +#include "utils/injection_point.h"
>
> 2) This can not only happen in error case but also in
On Mon, 14 Jul 2025 at 16:15, vignesh C wrote:
>
> On Mon, 14 Jul 2025 at 15:46, Ajin Cherian wrote:
> >
> > On Tue, Jul 8, 2025 at 8:41 PM Ajin Cherian wrote:
> > >
> > > Patch with fix attached.
> > > I'll continue investigating whether this issue also affects HEAD.
> > >
> >
> > While debuggi
On Mon, 14 Jul 2025 at 15:46, Ajin Cherian wrote:
>
> On Tue, Jul 8, 2025 at 8:41 PM Ajin Cherian wrote:
> >
> > Patch with fix attached.
> > I'll continue investigating whether this issue also affects HEAD.
> >
>
> While debugging if this problem can occur on HEAD, I found out that on
> head, it
On Tue, Jul 8, 2025 at 8:41 PM Ajin Cherian wrote:
>
> Patch with fix attached.
> I'll continue investigating whether this issue also affects HEAD.
>
While debugging if this problem can occur on HEAD, I found out that on
head, it is mostly the tablesync worker that drops the origin on HEAD
and si
On Tue, Jul 8, 2025 at 8:41 PM Ajin Cherian wrote:
> Proposed fix:
> In process_syncing_tables_for_apply(), acquire an AccessExclusiveLock
> on SubscriptionRelRelationId before acquiring the lock on
> ReplicationOriginRelationId.
>
> Patch with fix attached.
> I'll continue investigating whether
On Mon, Jul 7, 2025 at 8:15 PM Ajin Cherian wrote:
>
> On Sun, Jul 6, 2025 at 2:00 AM Alexander Lakhin wrote:
> >
> > --- a/src/backend/replication/logical/origin.c
> > +++ b/src/backend/replication/logical/origin.c
> > @@ -428,6 +428,7 @@ replorigin_drop_by_name(const char *name, bool
> > missi
On Sun, Jul 6, 2025 at 2:00 AM Alexander Lakhin wrote:
>
> --- a/src/backend/replication/logical/origin.c
> +++ b/src/backend/replication/logical/origin.c
> @@ -428,6 +428,7 @@ replorigin_drop_by_name(const char *name, bool
> missing_ok, bool nowait)
> * the specific origin and then re-
Hello hackers,
The recent buildfarm failure [1] on REL_15_STABLE with the following
diagnostics:
# Looks like your test exited with 29 just after 1.
t/024_add_drop_pub.pl ..
Dubious, test returned 29 (wstat 7424, 0x1d00)
pgsql.build/src/test/subscription/tmp_check/log/regress_log_024
36 matches
Mail list logo
