On Thu, Mar 29, 2018 at 05:15:14PM -0700, Andres Freund wrote:
> On 2018-03-22 15:45:23 +0000, Bossart, Nathan wrote:
>> Here is a set of patches for this approach.
>
> To me this looks like a reasonable approach that'd solve the VACUUM
> use-case. I think we can live with the API breakage, but I'd like to
> have some more comments? Pertinent parts quoted below.
I just looked at the proposed patches, that looks like a sensible
approach.
>> + /* verify that conflicting options are not specified */
>> + Assert((flags & (RELID_NOWAIT | RELID_SKIP_LOCKED)) != (RELID_NOWAIT | RELID_SKIP_LOCKED));
>> +
This is more readable as follows I think:
Assert((flags & RELID_NOWAIT) == 0 || (flags & RELID_SKIP_LOCKED) == 0);
>> /*
>> * We check the catalog name and then ignore it.
>> */
>> @@ -362,10 +372,13 @@ RangeVarGetRelidExtended(const RangeVar *relation, LOCKMODE lockmode,
>> */
>> if (!OidIsValid(relId))
>> AcceptInvalidationMessages();
>> - else if ((flags & RELID_NOWAIT) == 0)
>> + else if ((flags & (RELID_NOWAIT | RELID_SKIP_LOCKED)) == 0)
>> LockRelationOid(relId, lockmode);
>> else if (!ConditionalLockRelationOid(relId, lockmode))
>> {
>> + if ((flags & RELID_SKIP_LOCKED) != 0)
>> + return InvalidOid;
>> +
>> if (relation->schemaname)
>> ereport(ERROR,
>> (errcode(ERRCODE_LOCK_NOT_AVAILABLE),
That looks correct to me.
I would suggest to use uint8, uint16 or uint32 for the flags of
RangeVarGetRelidExtended instead of int. That's the practice in other
similar APIs with control flags.
+ * Note that if RELID_MISSING_OK and RELID_SKIP_LOCKED are both specified, a return
+ * value of InvalidOid could either mean the relation is missing or it could not be
+ * locked.
Perhaps we could generate a DEBUG message to help with making the
difference for developers?
So, +1 to simplify and break the interface.
--
Michael
