Re: Add “FOR UPDATE NOWAIT” lock details to the log.

> Thanks for the patch! Could you add it to the next CommitFest so we
> don't forget about it?
> 
>   RVR_MISSING_OK = 1 << 0, /* don't error if relation doesn't exist */
>   RVR_NOWAIT = 1 << 1, /* error if relation cannot be locked */
>   RVR_SKIP_LOCKED = 1 << 2, /* skip if relation cannot be locked */
> + RVR_LOG_LOCK_FAILURE = 1 << 3, /* log failure if relation cannot be locked */
>  } RVROption;
> 
> I understand this approach. But RVROption currently controls
> lookup/locking behavior such as MISSING_OK, NOWAIT, and SKIP_LOCKED,
> while RVR_LOG_LOCK_FAILURE is different in nature because it controls
> logging side effects rather than lookup/lock semantics themselves.
> 
> Because of that, it feels a bit out of place in this enum and makes
> the API boundary less clear. So I'd prefer to avoid adding
> RVR_LOG_LOCK_FAILURE to RVROption.
> 
> Instead, how about a simpler approach: update
> ConditionalLockRelationOid() to pass log_lock_failures to
> LockAcquireExtended()? That would allow not only LOCK TABLE NOWAIT,
> but also VACUUM SKIP LOCKED, ALTER ALL IN TABLESPACE NOWAIT, and
> REPACK to follow log_lock_failures.
> 
> Of course, that would also cause every autovacuum relation lock failure to be
> logged, which would likely be too noisy. To avoid that, how about
> forcibly disabling log_lock_failures in autovacuum workers? We already
> override some parameters there, so perhaps we could add something like
> this as well:
> 
>     /*
>      * Force log_lock_failures OFF in autovacuum workers to avoid verbose
>      * logging when maintenance skips contended relations.
>     */
>     SetConfigOption("log_lock_failures", "false", PGC_SUSET, PGC_S_OVERRIDE);
> 
> I think this approach is simpler. Thoughts?

Yeah, it's simpler, but it might still be too noisy if user do a whole-database
REPACK/VACUUM SKIP LOCKED. Do we need to care about this? If not, I think
your approach is in the right way.

My thought is to support LOCK TABLE NOWAIT only (other NOWAIT commands
can also be supported easily if we need). I agree that RVR_LOG_LOCK_FAILURE is
different from others, so I remove it in the v2 patch. Instead, only log lock failure
when RVR_NOWAIT is set. This avoids noisy logs because we will error out if we
cannot get the lock. Currently only LOCK TABLE NOWAIT uses the RVR_NOWAIT
flag.

I created a CF entry for this:

https://commitfest.postgresql.org/patch/6883/

--
Regards,
ChangAo Chen