Re: [HACKERS] [PATCH] SortSupport for macaddr type - Mailing list pgsql-hackers

From Brandur Leach
Subject Re: [HACKERS] [PATCH] SortSupport for macaddr type
Date
Msg-id CABR_9B__UJJjDckpEPJuuFsQMoVBYOLOPKNS7Rjh3NZGa7BJJA@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] [PATCH] SortSupport for macaddr type  (Julien Rouhaud <julien.rouhaud@dalibo.com>)
Responses Re: [HACKERS] [PATCH] SortSupport for macaddr type
Re: [HACKERS] [PATCH] SortSupport for macaddr type
Re: [PATCH] SortSupport for macaddr type
List pgsql-hackers
Hi Julien,

Thanks for the expedient reply, even after I'd dropped the
ball for so long :)

> Indeed, I should have checked more examples :/ There
> isn't any clear pattern for this, so I guess any one
> would be ok.

Yeah, agreed. If it's alright with you, I ended up moving
the naming back to `macaddr_cmp_internal` just because it
results in a smaller final diff.

> Thanks. I'm ok with this, but maybe a native english
> speaker would have a better opinion on this.

Cool! I just re-read my own comment a few days later and I
think that it still mostly makes sense, but definitely open
to other edits if anyone else has one.

> One last thing, I noticed that you added:
>
> +static int macaddr_internal_cmp(macaddr *a1, macaddr *a2);
>
> but the existing function is declared as
>
> static int32
> macaddr_internal_cmp(macaddr *a1, macaddr *a2)
>
> I'd be in favor to declare both as int.

Great catch! I have no idea how I missed that. I've done as
you suggested and made them both "int", which seems
consistent with SortSupport implementations elsewhere.

> After this, I think this patch will be ready for committer.

Excellent! I've attached a new (and hopefully final)
version of the patch.

Two final questions about the process if you'd be so kind:

* Should I change the status on the Commitfest link [1] or
  do I leave that to you (or someone else like a committer)?

* I've been generating a new OID value with the
  `unused_oids` script, but pretty much every time I rebase
  I collide with someone else's addition and need to find a
  new one. Is it better for me to pick an OID in an exotic
  range for my final patch, or that a committer just finds
  a new one (if necessary) as they're putting it into
  master?

Thanks again!
Brandur



On Sat, Feb 25, 2017 at 9:56 AM, Julien Rouhaud <julien.rouhaud@dalibo.com> wrote:
On Sun, Feb 05, 2017 at 01:56:41PM -0800, Brandur Leach wrote:

Hello Brandur, thanks for the updated patch!

>
> > * you used macaddr_cmp_internal() function name, for uuid
> >   the same function is named uuid_internal_cmp().  Using
> >   the same naming pattern is probably better.
>
> I was a little split on this one! It's true that UUID uses
> `_internal_cmp`, but `_cmp_internal` is also used in a
> number of places like `enum`, `timetz`, and `network`. I
> don't have a strong feeling about it either way, so I've
> changed it to `_internal_cmp` to match UUID as you
> suggested.
>

Indeed, I should have checked more examples :/ There isn't any clear pattern
for this, so I guess any one would be ok.

> > * the function comment on macaddr_abbrev_convert()
> >   doesn't mention specific little-endian handling
>
> I tried to bake this into the comment text. Here are the
> relevant lines of the amended version:
>
>     * Packs the bytes of a 6-byte MAC address into a Datum and treats it as
> an
>     * unsigned integer for purposes of comparison. On a 64-bit machine,
> there
>     * will be two zeroed bytes of padding. The integer is converted to
> native
>     * endianness to facilitate easy comparison.
>
> > * "There will be two bytes of zero padding on the least
> >   significant end"
> >
> > "least significant bits" would be better
>
> Also done. Here is the amended version:
>
>     * On a 64-bit machine, zero out the 8-byte datum and copy the 6 bytes of
>     * the MAC address in. There will be two bytes of zero padding on the end
>     * of the least significant bits.
>

Thanks.  I'm ok with this, but maybe a native english speaker would have a
better opinion on this.

> > * This patch will trigger quite a lot modifications after
> >   a pgindent run.  Could you try to run pgindent on mac.c
> >   before sending an updated patch?
>
> Good call! I've run the new version through pgindent.
>

Thanks also, no more issue here.

> Let me know if you have any further feedback and/or
> suggestions. Thanks!

One last thing, I noticed that you added:

+static int macaddr_internal_cmp(macaddr *a1, macaddr *a2);

but the existing function is declared as

static int32
macaddr_internal_cmp(macaddr *a1, macaddr *a2)

I'd be in favor to declare both as int.

After this, I think this patch will be ready for committer.

Attachment

pgsql-hackers by date:

Previous
From: Stephen Frost
Date:
Subject: Re: [HACKERS] Allow pg_dumpall to work without pg_authid
Next
From: Tomas Vondra
Date:
Subject: [HACKERS] update comments about CatalogUpdateIndexes