Thread: PublicationActions - use bit flags.

PublicationActions - use bit flags.

From
Peter Smith
Date:
For some reason the current HEAD PublicationActions is a struct of
boolean representing combinations of the 4 different "publication
actions".

I felt it is more natural to implement boolean flag combinations using
a bitmask instead of a struct of bools. IMO using the bitmask also
simplifies assignment and checking of said flags.

PSA a small patch for this.

Thoughts?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Attachment

Re: PublicationActions - use bit flags.

From
Greg Nancarrow
Date:
On Mon, Dec 20, 2021 at 11:19 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> For some reason the current HEAD PublicationActions is a struct of
> boolean representing combinations of the 4 different "publication
> actions".
>
> I felt it is more natural to implement boolean flag combinations using
> a bitmask instead of a struct of bools. IMO using the bitmask also
> simplifies assignment and checking of said flags.
>
> PSA a small patch for this.
>
> Thoughts?
>

+1
I think the bit flags are a more natural fit, and also the patch
removes the unnecessary use of a palloc'd tiny struct to return the
PublicationActions (which also currently isn't explicitly pfree'd).


Regards,
Greg Nancarrow
Fujitsu Australia



Re: PublicationActions - use bit flags.

From
Peter Eisentraut
Date:
On 20.12.21 01:18, Peter Smith wrote:
> For some reason the current HEAD PublicationActions is a struct of
> boolean representing combinations of the 4 different "publication
> actions".
> 
> I felt it is more natural to implement boolean flag combinations using
> a bitmask instead of a struct of bools. IMO using the bitmask also
> simplifies assignment and checking of said flags.

I don't see why this is better.  It just makes the code longer and adds 
more punctuation and reduces type safety.



Re: PublicationActions - use bit flags.

From
Tom Lane
Date:
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 20.12.21 01:18, Peter Smith wrote:
>> I felt it is more natural to implement boolean flag combinations using
>> a bitmask instead of a struct of bools. IMO using the bitmask also
>> simplifies assignment and checking of said flags.

> I don't see why this is better.  It just makes the code longer and adds 
> more punctuation and reduces type safety.

It makes the code shorter in places where you need to process all the
flags at once, but I agree it's not really an improvement elsewhere.
Not sure if it's worth changing.

One thing I noted is that the duplicate PublicationActions typedefs
will certainly draw warnings, if not hard errors, from some compilers.
You could get around that by removing the typedefs altogether and just
using "int", which'd be more consistent with our usual practices anyway.
But it does play into Peter's objection about type safety.

            regards, tom lane



Re: PublicationActions - use bit flags.

From
Alvaro Herrera
Date:
On 2021-Dec-20, Peter Eisentraut wrote:

> I don't see why this is better.  It just makes the code longer and adds more
> punctuation and reduces type safety.

Removing one palloc is I think the most important consequence ...
probably not a big deal though.

I think we could change the memcpy calls to struct assignment, as that
would look a bit cleaner, and call it a day.

One thing I would not like would be to change the catalog representation
from bools into an integer.  We do that for pg_trigger.tgflags (IIRC)
and it is horrible.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/



Re: PublicationActions - use bit flags.

From
Tom Lane
Date:
Greg Nancarrow <gregn4422@gmail.com> writes:
> I've attached a patch which addresses that and replaces a couple of
> memcpy()s with struct assignment, as suggested.

Removing this is not good:

     if (relation->rd_pubactions)
-    {
         pfree(relation->rd_pubactions);
-        relation->rd_pubactions = NULL;
-    }
 
If the subsequent palloc fails, you've created a problem where
there was none before.

I do wonder why we have to palloc a constant-size substructure in
the first place, especially one that is likely smaller than the
pointer that points to it.  Maybe the struct definition should be
moved so that we can just declare it in-line in the relcache entry?

            regards, tom lane



Re: PublicationActions - use bit flags.

From
Greg Nancarrow
Date:
On Tue, Dec 21, 2021 at 11:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Removing this is not good:
>
>         if (relation->rd_pubactions)
> -       {
>                 pfree(relation->rd_pubactions);
> -               relation->rd_pubactions = NULL;
> -       }
>
> If the subsequent palloc fails, you've created a problem where
> there was none before.
>

Oops, yeah, I got carried away; if palloc() failed and called exit(),
then it would end up crashing when trying to use/pfree rd_pubactions
again.
Better leave that line in ...

> I do wonder why we have to palloc a constant-size substructure in
> the first place, especially one that is likely smaller than the
> pointer that points to it.  Maybe the struct definition should be
> moved so that we can just declare it in-line in the relcache entry?
>

I think currently it's effectively using the rd_pubactions pointer as
a boolean flag to indicate whether the publication membership info has
been fetched (so the bool flags are valid).
I guess you'd need another bool flag if you wanted to make that struct
in-line in the relcache entry.


Regards,
Greg Nancarrow
Fujitsu Australia



Re: PublicationActions - use bit flags.

From
Peter Smith
Date:
On Tue, Dec 21, 2021 at 11:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Greg Nancarrow <gregn4422@gmail.com> writes:
> > I've attached a patch which addresses that and replaces a couple of
> > memcpy()s with struct assignment, as suggested.
>
> Removing this is not good:
>
>         if (relation->rd_pubactions)
> -       {
>                 pfree(relation->rd_pubactions);
> -               relation->rd_pubactions = NULL;
> -       }
>
> If the subsequent palloc fails, you've created a problem where
> there was none before.
>
> I do wonder why we have to palloc a constant-size substructure in
> the first place, especially one that is likely smaller than the
> pointer that points to it.  Maybe the struct definition should be
> moved so that we can just declare it in-line in the relcache entry?
>

At the risk of flogging a dead horse, here is v2 of my original
bit-flag replacement for the PublicationActions struct.

This version introduces one more bit flag for the relcache status, and
by doing so means all that code for Relation cache PublicationActions
pointers and pallocs and context switches can just disappear...

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Attachment

Re: PublicationActions - use bit flags.

From
Justin Pryzby
Date:
On Mon, Dec 20, 2021 at 11:18:41AM +1100, Peter Smith wrote:
> For some reason the current HEAD PublicationActions is a struct of
> boolean representing combinations of the 4 different "publication
> actions".
> 
> I felt it is more natural to implement boolean flag combinations using
> a bitmask instead of a struct of bools. IMO using the bitmask also
> simplifies assignment and checking of said flags.

Peter E made a suggestion to use a similar struct with bools last year for
REINDEX.
https://www.postgresql.org/message-id/7ec67c56-2377-cd05-51a0-691104404abe@enterprisedb.com

Alvaro pointed out that the integer flags are better for ABI compatibility - it
would allow adding a new flag in backbranches, if that were needed for a
bugfix.

But that's not very compelling, since the bools have existed in v10...
Also, the booleans directly correspond with the catalog.

+       if (pubform->pubinsert) pub->pubactions |= PUBACTION_INSERT;

This is usually written like:

    pub->pubactions |= (pubform->pubinsert ? PUBACTION_INSERT : 0)

-- 
Justin



Re: PublicationActions - use bit flags.

From
Peter Smith
Date:
On Thu, Dec 30, 2021 at 3:30 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, Dec 20, 2021 at 11:18:41AM +1100, Peter Smith wrote:
> > For some reason the current HEAD PublicationActions is a struct of
> > boolean representing combinations of the 4 different "publication
> > actions".
> >
> > I felt it is more natural to implement boolean flag combinations using
> > a bitmask instead of a struct of bools. IMO using the bitmask also
> > simplifies assignment and checking of said flags.
>
> Peter E made a suggestion to use a similar struct with bools last year for
> REINDEX.
> https://www.postgresql.org/message-id/7ec67c56-2377-cd05-51a0-691104404abe@enterprisedb.com
>
> Alvaro pointed out that the integer flags are better for ABI compatibility - it
> would allow adding a new flag in backbranches, if that were needed for a
> bugfix.
>
> But that's not very compelling, since the bools have existed in v10...
> Also, the booleans directly correspond with the catalog.
>
> +       if (pubform->pubinsert) pub->pubactions |= PUBACTION_INSERT;
>
> This is usually written like:
>
>         pub->pubactions |= (pubform->pubinsert ? PUBACTION_INSERT : 0)
>

Thanks for the info, I've modified those assignment styles as suggested.

------
Kind Regards,
Peter Smith.
Fujitsu Australia.

Attachment

Re: PublicationActions - use bit flags.

From
Tom Lane
Date:
Peter Smith <smithpb2250@gmail.com> writes:
> On Thu, Dec 30, 2021 at 3:30 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>> +       if (pubform->pubinsert) pub->pubactions |= PUBACTION_INSERT;
>> This is usually written like:
>> pub->pubactions |= (pubform->pubinsert ? PUBACTION_INSERT : 0)

> Thanks for the info, I've modified those assignment styles as suggested.

FWIW, I think it's utter nonsense to claim that the second way is
preferred over the first.  There may be some people who think
the second way is more legible, but I don't; and I'm pretty sure
that the first way is significantly more common in the PG codebase.

            regards, tom lane



Re: PublicationActions - use bit flags.

From
Greg Nancarrow
Date:
On Tue, Dec 21, 2021 at 12:55 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Tue, Dec 21, 2021 at 11:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Removing this is not good:
> >
> >         if (relation->rd_pubactions)
> > -       {
> >                 pfree(relation->rd_pubactions);
> > -               relation->rd_pubactions = NULL;
> > -       }
> >
> > If the subsequent palloc fails, you've created a problem where
> > there was none before.
> >
>
> Oops, yeah, I got carried away; if palloc() failed and called exit(),
> then it would end up crashing when trying to use/pfree rd_pubactions
> again.
> Better leave that line in ...
>

Attaching an updated patch to fix that oversight.
This patch thus fixes the original palloc issue in a minimal way,
keeping the same relcache structure.

Regards,
Greg Nancarrow
Fujitsu Australia

Attachment

Re: PublicationActions - use bit flags.

From
Peter Eisentraut
Date:
On 21.01.22 01:05, Greg Nancarrow wrote:
> On Tue, Dec 21, 2021 at 12:55 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>>
>> On Tue, Dec 21, 2021 at 11:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>
>>> Removing this is not good:
>>>
>>>          if (relation->rd_pubactions)
>>> -       {
>>>                  pfree(relation->rd_pubactions);
>>> -               relation->rd_pubactions = NULL;
>>> -       }
>>>
>>> If the subsequent palloc fails, you've created a problem where
>>> there was none before.
>>>
>>
>> Oops, yeah, I got carried away; if palloc() failed and called exit(),
>> then it would end up crashing when trying to use/pfree rd_pubactions
>> again.
>> Better leave that line in ...
>>
> 
> Attaching an updated patch to fix that oversight.
> This patch thus fixes the original palloc issue in a minimal way,
> keeping the same relcache structure.

Why can't GetRelationPublicationActions() have the PublicationActions as 
a return value, instead of changing it to an output argument?



Re: PublicationActions - use bit flags.

From
Greg Nancarrow
Date:

On Tue, Jan 25, 2022 at 7:31 AM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> Why can't GetRelationPublicationActions() have the PublicationActions as
> a return value, instead of changing it to an output argument?

That would be OK too, for now, for the current (small size, typically 4-byte) PublicationActions struct.
But if that function was extended in the future to return more publication information than just the PublicationActions struct (and I'm seeing that in the filtering patches [1]), then using return-by-value won't be as efficient as pass-by-reference, and I'd tend to stick with pass-by-reference in that case.


Regards,
Greg Nancarrow
Fujitsu Australia

Re: PublicationActions - use bit flags.

From
Peter Eisentraut
Date:
On 25.01.22 07:14, Greg Nancarrow wrote:
> 
> On Tue, Jan 25, 2022 at 7:31 AM Peter Eisentraut 
> <peter.eisentraut@enterprisedb.com 
> <mailto:peter.eisentraut@enterprisedb.com>> wrote:
>  >
>  > Why can't GetRelationPublicationActions() have the PublicationActions as
>  > a return value, instead of changing it to an output argument?
> 
> That would be OK too, for now, for the current (small size, typically 
> 4-byte) PublicationActions struct.
> But if that function was extended in the future to return more 
> publication information than just the PublicationActions struct (and I'm 
> seeing that in the filtering patches [1]), then using return-by-value 
> won't be as efficient as pass-by-reference, and I'd tend to stick with 
> pass-by-reference in that case.
> 
> [1] 
> https://postgr.es/m/OS0PR01MB5716B899A66D2997EF28A1B3945F9%40OS0PR01MB5716.jpnprd01.prod.outlook.com 
> <https://postgr.es/m/OS0PR01MB5716B899A66D2997EF28A1B3945F9%40OS0PR01MB5716.jpnprd01.prod.outlook.com>

By itself, this refactoring doesn't seem worth it.  The code is actually 
longer at the end, and we haven't made it any more extensible or 
anything.  And AFAICT, this is not called in a performance-sensitive way.

The proposed changes in [1] change this function more significantly, so 
adopting the present change wouldn't really help there either except 
create the need for one more rebase.

So I think we should leave this alone here and let [1] make the changes 
it needs.