On Fri, Sep 25, 2026 at 7:57 AM Álvaro Herrera <[email protected]> wrote:
>
> On 2026-Sep-25, Thom Brown wrote:
>
> > Moving it into cluster_rel() makes sense, but does it need to be the
> > full ShareUpdateExclusiveLock there?
>
> Yeah, it's better to acquire the lock you want upfront, because
> otherwise you introduce more risk of deadlock caused by lock upgrades
> (admittedly the user would have to be doing something really stupid in
> order for this to be a real problem, but still.) It's only AEL that we
> don't want to hold for long.
+1
> Maybe the patch could be somewhat like this, then? I didn't review the
> test carefully other than running without the code fix to verify that it
> fails, and then passes with the fix; and I didn't read the commit
> messages either, which I think are LLM-written and not really correct.
> (Also, I would push both things as a single commit.)
The patch looks good to me. How about updating the comment in
repack_setup_logical_decoding() to explain that the backend holds the
lock on both relations so the worker doesn't need to keep the locks on
them? For example:
@@ -253,7 +253,9 @@ repack_setup_logical_decoding(Oid relid)
/*
* Set up repacked_rel_locator and repacked_rel_toast_locator, which we
- * use to skip decoding of unrelated relations.
+ * use to skip decoding of unrelated relations. We need not keep the
+ * locks as the backend holds a lock on both the table and its TOAST
+ * relation that prevents them from being rewritten until REPACK finishes.
*/
rel = table_open(relid, AccessShareLock);
repacked_rel_locator = rel->rd_locator;
> I think changing the lock as obtained by copy_table_data is not very
> nice, because that one is unconditional, and here we only want it in
> concurrent mode. BTW I noticed that the comment for copy_table_data
> mentions decoding_ctx as an argument, which doesn't exist.
True.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
