On 2018-03-30 11:37:19 +0900, Michael Paquier wrote:
> 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);
I found Assert(!((flags & RELID_NOWAIT) && (flags & RELID_SKIP_LOCKED)))
easier. I've removed a number of of parens from, in my opinion,
over-parenthized statements.
On 2018-03-30 14:55:45 -0700, Andres Freund wrote:
> On 2018-03-30 17:08:26 +0000, Bossart, Nathan wrote:
> > +typedef enum RelidOption
> > +{
> > + RELID_MISSING_OK = 1 << 0, /* don't error if relation doesn't
> > exist */
> > + RELID_NOWAIT = 1 << 1 /* error if relation cannot be locked */
> > +} RelidOption;
>
> I don't like the Relid prefix here. RangeVarGetRelid deals with
> *rangevars*, and returns a relation oid. ISTM it'd be more accurate to
> call this RV_* or RVID_*. Counterarguments, preferences?
Went with RVR_*
Pushed.
Greetings,
Andres Freund
