> 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
v2-0001-Make-log_lock_failures-support-LOCK-NOWAIT.patch
Description: Binary data
