Thread: Locks on unlogged tables are locked?!

While looking at this:
https://stackoverflow.com/q/50413623/6464308
I realized that "LOCK TABLE <unlogged table>" puts a
Standby/LOCK into the WAL stream, which causes a log flush
at COMMIT time.

That hurts performance, and I doubt that it is necessary.
At any rate, DROP TABLE on an unlogged table logs nothing.

The attached patch would take care of it, but I'm not sure
if that's the right place to check.

Yours,
Laurenz Albe

Uh, was this ever addressed?  I don't see the patch applied or the code
in this area modified.

---------------------------------------------------------------------------

On Thu, May 24, 2018 at 04:33:11PM +0200, Laurenz Albe wrote:
> While looking at this:
> https://stackoverflow.com/q/50413623/6464308
> I realized that "LOCK TABLE <unlogged table>" puts a
> Standby/LOCK into the WAL stream, which causes a log flush
> at COMMIT time.
> 
> That hurts performance, and I doubt that it is necessary.
> At any rate, DROP TABLE on an unlogged table logs nothing.
> 
> The attached patch would take care of it, but I'm not sure
> if that's the right place to check.
> 
> Yours,
> Laurenz Albe

> From d474e7b41298944e43bb9141eb33adbdd9ea1098 Mon Sep 17 00:00:00 2001
> From: Laurenz Albe <laurenz.albe@cybertec.at>
> Date: Tue, 22 May 2018 18:13:31 +0200
> Subject: [PATCH] Don't log locks on unlogged tables
> 
> ---
>  src/backend/storage/lmgr/lock.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
> index dc3d8d9817..70cac47ab3 100644
> --- a/src/backend/storage/lmgr/lock.c
> +++ b/src/backend/storage/lmgr/lock.c
> @@ -37,6 +37,7 @@
>  #include "access/twophase_rmgr.h"
>  #include "access/xact.h"
>  #include "access/xlog.h"
> +#include "catalog/pg_class.h"
>  #include "miscadmin.h"
>  #include "pg_trace.h"
>  #include "pgstat.h"
> @@ -47,6 +48,7 @@
>  #include "storage/standby.h"
>  #include "utils/memutils.h"
>  #include "utils/ps_status.h"
> +#include "utils/rel.h"
>  #include "utils/resowner_private.h"
>  
>  
> @@ -1041,13 +1043,25 @@ LockAcquireExtended(const LOCKTAG *locktag,
>       */
>      if (log_lock)
>      {
> -        /*
> -         * Decode the locktag back to the original values, to avoid sending
> -         * lots of empty bytes with every message.  See lock.h to check how a
> -         * locktag is defined for LOCKTAG_RELATION
> -         */
> -        LogAccessExclusiveLock(locktag->locktag_field1,
> -                               locktag->locktag_field2);
> +        bool unlogged_rel = false;
> +
> +        if (locktag->locktag_type == LOCKTAG_RELATION)
> +        {
> +            Relation r = RelationIdGetRelation(locktag->locktag_field2);
> +            unlogged_rel = r->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED;
> +            RelationClose(r);
> +        }
> +
> +        if (!unlogged_rel)
> +        {
> +            /*
> +             * Decode the locktag back to the original values, to avoid sending
> +             * lots of empty bytes with every message.  See lock.h to check how a
> +             * locktag is defined for LOCKTAG_RELATION
> +             */
> +            LogAccessExclusiveLock(locktag->locktag_field1,
> +                                   locktag->locktag_field2);
> +        }
>      }
>  
>      return LOCKACQUIRE_OK;
> -- 
> 2.14.3
> 


-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Tue, Nov 21, 2023 at 11:49 AM Bruce Momjian <bruce@momjian.us> wrote:
> Uh, was this ever addressed?  I don't see the patch applied or the code
> in this area modified.

I never saw this email originally, but I don't think I believe
Laurenz's argument. Are all AEL-requiring operations on unlogged
tables safe to perform on a standby without AEL? I believe I would be
quite surprised if that turned out to be true.

--
Robert Haas
EDB: http://www.enterprisedb.com

Bruce Momjian <bruce@momjian.us> writes:
> Uh, was this ever addressed?  I don't see the patch applied or the code
> in this area modified.

This patch as-is would surely be disastrous: having LockAcquire
try to open the relcache entry for the thing we're trying to lock
is going to be circular in at least some cases.  I'm not convinced
that there's a problem worth solving here, but if there is, it'd
have to be done in some other way.

            regards, tom lane

On Tue, Nov 21, 2023 at 01:16:19PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Uh, was this ever addressed?  I don't see the patch applied or the code
> > in this area modified.
> 
> This patch as-is would surely be disastrous: having LockAcquire
> try to open the relcache entry for the thing we're trying to lock
> is going to be circular in at least some cases.  I'm not convinced
> that there's a problem worth solving here, but if there is, it'd
> have to be done in some other way.

Thank you, and Robert, for the feedback.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.