Thread: Avoid unecessary MemSet call (src/backend/utils/cache/relcache.c)

On Sun, 15 May 2022 at 09:47, Ranier Vilela <ranier.vf@gmail.com> wrote:
> At function load_relcache_init_file, there is an unnecessary function call,
> to initialize pgstat_info pointer to NULL.
>
> MemSet(&rel->pgstat_info, 0, sizeof(rel->pgstat_info));

What seems to have happened here is the field was changed to become a
pointer in 77947c51c.  It's not incorrect to use MemSet() to zero out
the pointer field.  What it does probably do is confuse the casual
reader into thinking the field is a struct rather than a pointer to
one.   It's probably worth making that consistent with the other
fields so nobody gets confused.

Can you add a CF entry for PG16 for this so we come back to it after we branch?

David

On Tue, May 17, 2022 at 07:52:30PM -0300, Ranier Vilela wrote:
> I found, I believe, a serious problem of incorrect usage of the memset api.
> Historically, people have relied on using memset or MemSet, using the
> variable name as an argument for the sizeof.
> While it works correctly, for arrays, when it comes to pointers to
> structures, things go awry.

Knowing how sizeof() works is required before using it - the same is true for
pointers.

> So throughout the code there are these misuses.

Why do you think it's a misuse ?

Take the first one as an example.  It says:

        GenericCosts costs;
        MemSet(&costs, 0, sizeof(costs));

You sent a patch to change it to sizeof(GenericCosts).

But it's not a pointer, so they are the same.

Is that true for every change in your patch ?

-- 
Justin

Ranier Vilela <ranier.vf@gmail.com> writes:
> I found, I believe, a serious problem of incorrect usage of the memset api.
> Historically, people have relied on using memset or MemSet, using the
> variable name as an argument for the sizeof.
> While it works correctly, for arrays, when it comes to pointers to
> structures, things go awry.

You'll have to convince people that any of these places are in
fact incorrect.  Everyone who's used C for any length of time
is well aware of the possibility of getting sizeof() wrong in
this sort of context, and I think we've been careful about it.

Also, as a stylistic matter I think it's best to write
"memset(&x, 0, sizeof(x))" where we can.  Replacing sizeof(x)
with sizeof(some type name) has its own risks of error, and
therefore is not automatically an improvement.

            regards, tom lane

This one caught my attention:

diff --git a/contrib/pgcrypto/crypt-blowfish.c b/contrib/pgcrypto/crypt-blowfish.c
index a663852ccf..63fcef562d 100644
--- a/contrib/pgcrypto/crypt-blowfish.c
+++ b/contrib/pgcrypto/crypt-blowfish.c
@@ -750,7 +750,7 @@ _crypt_blowfish_rn(const char *key, const char *setting,
 /* Overwrite the most obvious sensitive data we have on the stack. Note
  * that this does not guarantee there's no sensitive data left on the
  * stack and/or in registers; I'm not aware of portable code that does. */
-    px_memset(&data, 0, sizeof(data));
+    px_memset(&data, 0, sizeof(struct data));
 
     return output;
 }

The curious thing here is that sizeof(data) is correct, because it
refers to a variable defined earlier in that function, whose type is an
anonymous struct declared there.  But I don't know what "struct data"
refers to, precisely because that struct is unnamed.  Am I misreading it?


Also:

diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index e1048e47ff..87be62f023 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -601,7 +601,7 @@ pgstathashindex(PG_FUNCTION_ARGS)
                  errmsg("cannot access temporary indexes of other sessions")));
 
     /* Get the information we need from the metapage. */
-    memset(&stats, 0, sizeof(stats));
+    memset(&stats, 0, sizeof(HashIndexStat));
     metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
     metap = HashPageGetMeta(BufferGetPage(metabuf));
     stats.version = metap->hashm_version;

I think the working theory here is that the original line is correct
now, and it continues to be correct if somebody edits the function and
makes variable 'stats' be of a different type.  But if you change the
sizeof() to use the type name, then there are two places that you need
to edit, and they are not necessarily close together; so it is correct
now and could become a bug in the future.  I don't think we're fully
consistent about this, but I think you're proposing to change it in the
opposite direction that we'd prefer.

For the case where the variable is a pointer, the developer could write
'sizeof(*variable)' instead of being forced to specify the type name,
for example (just a random one):

diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index a434cf93ef..e92c03686f 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -438,7 +438,7 @@ BloomFillMetapage(Relation index, Page metaPage)
      */
     BloomInitPage(metaPage, BLOOM_META);
     metadata = BloomPageGetMeta(metaPage);
-    memset(metadata, 0, sizeof(BloomMetaPageData));
+    memset(metadata, 0, sizeof(*metadata));
     metadata->magickNumber = BLOOM_MAGICK_NUMBER;
     metadata->opts = *opts;
     ((PageHeader) metaPage)->pd_lower += sizeof(BloomMetaPageData);

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

On 18.05.22 01:18, Justin Pryzby wrote:
> Take the first one as an example.  It says:
> 
>          GenericCosts costs;
>          MemSet(&costs, 0, sizeof(costs));
> 
> You sent a patch to change it to sizeof(GenericCosts).
> 
> But it's not a pointer, so they are the same.

This instance can more easily be written as

     costs = {0};

On Thu, 19 May 2022 at 02:08, Ranier Vilela <ranier.vf@gmail.com> wrote:
> That would initialize the content at compilation and not at runtime, correct?

Your mental model of compilation and run-time might be flawed here.
Here's no such thing as zeroing memory at compile time. There's only
emitting instructions that perform those tasks at run-time.
https://godbolt.org/ might help your understanding.

> There are a lot of cases using MemSet (with struct variables) and at Windows 64 bits, long are 4 (four) bytes.
> So I believe that MemSet is less efficient on Windows than on Linux.
> "The size of the '_vstart' buffer is not a multiple of the element size of the type 'long'."
> message from PVS-Studio static analysis tool.

I've been wondering for a while if we really need to have the MemSet()
macro. I see it was added in 8cb415449 (1997).  I think compilers have
evolved quite a bit in the past 25 years, so it could be time to
revisit that.

Your comment on the sizeof(long) on win64 is certainly true.  I wrote
the attached C program to test the performance difference.

(windows 64-bit)
>cl memset.c /Ox
>memset 200000000
Running 200000000 loops
MemSet: size 8: 1.833000 seconds
MemSet: size 16: 1.841000 seconds
MemSet: size 32: 1.838000 seconds
MemSet: size 64: 1.851000 seconds
MemSet: size 128: 3.228000 seconds
MemSet: size 256: 5.278000 seconds
MemSet: size 512: 3.943000 seconds
memset: size 8: 0.065000 seconds
memset: size 16: 0.131000 seconds
memset: size 32: 0.262000 seconds
memset: size 64: 0.530000 seconds
memset: size 128: 1.169000 seconds
memset: size 256: 2.950000 seconds
memset: size 512: 3.191000 seconds

It seems like there's no cases there where MemSet is faster than
memset.  I was careful to only provide MemSet() with inputs that
result in it not using the memset fallback.  I also provided constants
so that the decision about which method to use was known at compile
time.

It's not clear to me why 512 is faster than 256. I saw the same on a repeat run.

Changing "long" to "long long" it looks like:

>memset 200000000
Running 200000000 loops
MemSet: size 8: 0.066000 seconds
MemSet: size 16: 1.978000 seconds
MemSet: size 32: 1.982000 seconds
MemSet: size 64: 1.973000 seconds
MemSet: size 128: 1.970000 seconds
MemSet: size 256: 3.225000 seconds
MemSet: size 512: 5.366000 seconds
memset: size 8: 0.069000 seconds
memset: size 16: 0.132000 seconds
memset: size 32: 0.265000 seconds
memset: size 64: 0.527000 seconds
memset: size 128: 1.161000 seconds
memset: size 256: 2.976000 seconds
memset: size 512: 3.179000 seconds

The situation is a little different on my Linux machine:

$ gcc memset.c -o memset -O2
$ ./memset 200000000
Running 200000000 loops
MemSet: size 8: 0.000002 seconds
MemSet: size 16: 0.000000 seconds
MemSet: size 32: 0.094041 seconds
MemSet: size 64: 0.184618 seconds
MemSet: size 128: 1.781503 seconds
MemSet: size 256: 2.547910 seconds
MemSet: size 512: 4.005173 seconds
memset: size 8: 0.046156 seconds
memset: size 16: 0.046123 seconds
memset: size 32: 0.092291 seconds
memset: size 64: 0.184509 seconds
memset: size 128: 1.781518 seconds
memset: size 256: 2.577104 seconds
memset: size 512: 4.004757 seconds

It looks like part of the work might be getting optimised away in the
8-16 MemSet() calls.

clang seems to have the opposite for size 8.

$ clang memset.c -o memset -O2
$ ./memset 200000000
Running 200000000 loops
MemSet: size 8: 0.007653 seconds
MemSet: size 16: 0.005771 seconds
MemSet: size 32: 0.011539 seconds
MemSet: size 64: 0.023095 seconds
MemSet: size 128: 0.046130 seconds
MemSet: size 256: 0.092269 seconds
MemSet: size 512: 0.968564 seconds
memset: size 8: 0.000000 seconds
memset: size 16: 0.005776 seconds
memset: size 32: 0.011559 seconds
memset: size 64: 0.023069 seconds
memset: size 128: 0.046129 seconds
memset: size 256: 0.092243 seconds
memset: size 512: 0.968534 seconds

There does not seem to be any significant reduction in the size of the
binary from changing the MemSet macro to directly use memset. It went
from 9865008 bytes down to 9860800 bytes (4208 bytes less).

David

David Rowley <dgrowleyml@gmail.com> writes:
> I've been wondering for a while if we really need to have the MemSet()
> macro. I see it was added in 8cb415449 (1997).  I think compilers have
> evolved quite a bit in the past 25 years, so it could be time to
> revisit that.

Yeah, I've thought for awhile that technology has moved on from that.
Nobody's really taken the trouble to measure it though.  (And no,
results from one compiler on one machine are not terribly convincing.)

The thing that makes this a bit more difficult than it might be is
the special cases we have for known-aligned and so on targets, which
are particularly critical for palloc0 and makeNode etc.  So there's
more than one case to look into.  But I'd argue that those special
cases are actually what we want to worry about the most: zeroing
relatively small, known-aligned node structs is THE use case.

            regards, tom lane

On Thu, 19 May 2022 at 11:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The thing that makes this a bit more difficult than it might be is
> the special cases we have for known-aligned and so on targets, which
> are particularly critical for palloc0 and makeNode etc.  So there's
> more than one case to look into.  But I'd argue that those special
> cases are actually what we want to worry about the most: zeroing
> relatively small, known-aligned node structs is THE use case.

I think the makeNode() trickery would be harder to get rid of, or for
that matter, anything where the size/alignment is unknown at compile
time.  I think the more interesting ones that we might be able to get
rid of are the ones where the alignment and size *are* known at
compile time. Also probably anything that passes a compile-time const
that's not 0 will fallback on memset anyway, so might as well be
removed to tidy things up.

It just all seems a bit untidy when you look at functions like
ExecStoreAllNullTuple() which use a mix of memset and MemSet without
any apparent explanation of why. That particular one is likely that
way due to the first size guaranteed to be multiples of sizeof(Datum)
and the latter not.

Naturally, we'd need to run enough benchmarks to prove to ourselves
that we're not causing any slowdowns.  The intention of memset.c was
to try to put something out there that people could test so we could
get an idea if there are any machines/compilers that we might need to
be concerned about.

David

On 19.05.22 18:09, Ranier Vilela wrote:
> Taking it a step further.
> Created a new patch into commitfest, targeting 16 version.
> https://commitfest.postgresql.org/38/3645/ 
> <https://commitfest.postgresql.org/38/3645/>

I have committed your 001 patch, which was clearly a (harmless) mistake.

I have also committed a patch that gets rid of MemSet() calls where the 
value is a constant not-0, because that just falls back to memset() anyway.

I'm on board with trying to get rid of MemSet(), but first I need to 
analyze all the performance numbers and arguments that were shown in 
this thread.

On 18.05.22 15:52, Peter Eisentraut wrote:
> On 18.05.22 01:18, Justin Pryzby wrote:
>> Take the first one as an example.  It says:
>>
>>          GenericCosts costs;
>>          MemSet(&costs, 0, sizeof(costs));
>>
>> You sent a patch to change it to sizeof(GenericCosts).
>>
>> But it's not a pointer, so they are the same.
> 
> This instance can more easily be written as
> 
>      costs = {0};

The attached patch replaces all MemSet() calls with struct 
initialization where that is easily possible.  (For example, some cases 
have to worry about padding bits, so I left those.)

This reduces the number of MemSet() calls from about 400 to about 200. 
Maybe this can help simplify the investigation of the merits of the 
remaining calls.

On 2022-Jul-07, Peter Eisentraut wrote:

> diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
> index 4445a86aee..79b23fa7d7 100644
> --- a/src/bin/pg_basebackup/pg_basebackup.c
> +++ b/src/bin/pg_basebackup/pg_basebackup.c

> @@ -1952,7 +1948,6 @@ BaseBackup(char *compression_algorithm, char *compression_detail,
>      else
>          starttli = latesttli;
>      PQclear(res);
> -    MemSet(xlogend, 0, sizeof(xlogend));
>  
>      if (verbose && includewal != NO_WAL)
>          pg_log_info("write-ahead log start point: %s on timeline %u",

You removed the MemSet here, but there's no corresponding
initialization.

> diff --git a/src/port/snprintf.c b/src/port/snprintf.c
> index abb1c59770..e646b0e642 100644
> --- a/src/port/snprintf.c
> +++ b/src/port/snprintf.c
> @@ -756,12 +756,9 @@ find_arguments(const char *format, va_list args,
>      int            longflag;
>      int            fmtpos;
>      int            i;
> -    int            last_dollar;
> -    PrintfArgType argtypes[PG_NL_ARGMAX + 1];
> -
>      /* Initialize to "no dollar arguments known" */
> -    last_dollar = 0;
> -    MemSet(argtypes, 0, sizeof(argtypes));
> +    int            last_dollar = 0;
> +    PrintfArgType argtypes[PG_NL_ARGMAX + 1] = {0};

pgindent will insert a blank line before the comment, which I personally
find quite ugly (because it splits the block of declarations).

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El Maquinismo fue proscrito so pena de cosquilleo hasta la muerte"
(Ijon Tichy en Viajes, Stanislaw Lem)

On 01.07.22 17:58, Ranier Vilela wrote:
> Em qui., 30 de jun. de 2022 às 19:37, Peter Eisentraut 
> <peter.eisentraut@enterprisedb.com 
> <mailto:peter.eisentraut@enterprisedb.com>> escreveu:
> 
>     I have also committed a patch that gets rid of MemSet() calls where the
>     value is a constant not-0, because that just falls back to memset()
>     anyway.
> 
> Peter there are some missing paths in this commit.

As I wrote in the commit message:

(There are a few MemSet() calls that I didn't change to maintain the 
consistency with their surrounding code.)

On 07.07.22 13:16, Alvaro Herrera wrote:
> On 2022-Jul-07, Peter Eisentraut wrote:
> 
>> diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
>> index 4445a86aee..79b23fa7d7 100644
>> --- a/src/bin/pg_basebackup/pg_basebackup.c
>> +++ b/src/bin/pg_basebackup/pg_basebackup.c
> 
>> @@ -1952,7 +1948,6 @@ BaseBackup(char *compression_algorithm, char *compression_detail,
>>       else
>>           starttli = latesttli;
>>       PQclear(res);
>> -    MemSet(xlogend, 0, sizeof(xlogend));
>>   
>>       if (verbose && includewal != NO_WAL)
>>           pg_log_info("write-ahead log start point: %s on timeline %u",
> 
> You removed the MemSet here, but there's no corresponding
> initialization.

Maybe that was an oversight by me, but it seems to me that that 
initialization was useless anyway, since xlogend is later 
unconditionally overwritten anyway.

>> diff --git a/src/port/snprintf.c b/src/port/snprintf.c
>> index abb1c59770..e646b0e642 100644
>> --- a/src/port/snprintf.c
>> +++ b/src/port/snprintf.c
>> @@ -756,12 +756,9 @@ find_arguments(const char *format, va_list args,
>>       int            longflag;
>>       int            fmtpos;
>>       int            i;
>> -    int            last_dollar;
>> -    PrintfArgType argtypes[PG_NL_ARGMAX + 1];
>> -
>>       /* Initialize to "no dollar arguments known" */
>> -    last_dollar = 0;
>> -    MemSet(argtypes, 0, sizeof(argtypes));
>> +    int            last_dollar = 0;
>> +    PrintfArgType argtypes[PG_NL_ARGMAX + 1] = {0};
> 
> pgindent will insert a blank line before the comment, which I personally
> find quite ugly (because it splits the block of declarations).

Yeah.  I think I can convert that to an end-of-line comment instead.

On 11.07.22 21:06, Ranier Vilela wrote:
> Em qui., 7 de jul. de 2022 às 14:01, Ranier Vilela <ranier.vf@gmail.com 
> <mailto:ranier.vf@gmail.com>> escreveu:
> 
>     Attached the v1 of your patch.
>     I think that all is safe to switch MemSet by {0}.
> 
> Here the rebased patch v2, against latest head.

I have committed my patch with Álvaro's comments addressed.

Your patch appears to add in changes that are either arguably out of 
scope or would need further review (e.g., changing memset() calls, 
changing the scope of some variables, changing places that need to worry 
about padding bits).  Please submit separate patches for those, and we 
can continue the analysis.

On Tue, Aug 2, 2022 at 3:09 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Em sáb., 16 de jul. de 2022 às 16:54, Ranier Vilela <ranier.vf@gmail.com> escreveu:
>>
>>
>>
>> Em sáb, 16 de jul de 2022 2:58 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> escreveu:
>>>
>>> On 11.07.22 21:06, Ranier Vilela wrote:
>>> > Em qui., 7 de jul. de 2022 às 14:01, Ranier Vilela <ranier.vf@gmail.com
>>> > <mailto:ranier.vf@gmail.com>> escreveu:
>>> >
>>> >     Attached the v1 of your patch.
>>> >     I think that all is safe to switch MemSet by {0}.
>>> >
>>> > Here the rebased patch v2, against latest head.
>>>
>>> I have committed my patch with Álvaro's comments addressed
>>
>> I see.
>> It's annoing that old compiler (gcc 4.7.2) don't handle this style.
>>
>>>
>>> Your patch appears to add in changes that are either arguably out of
>>> scope or would need further review (e.g., changing memset() calls,
>>> changing the scope of some variables, changing places that need to worry
>>> about padding bits).  Please submit separate patches for those, and we
>>> can continue the analysis.
>>
>> Sure.
>
> Hi, sorry for the delay.
> Like how https://github.com/postgres/postgres/commit/9fd45870c1436b477264c0c82eb195df52bc0919
> New attempt to remove more MemSet calls, that are safe.
>
> Attached v3 patch.
>
> regards,
> Ranier Vilela

Hi, I have not been closely following this thread, but it's starting
to sound very deja-vu with something I proposed 3 years ago. See [1]
"Make use of C99 designated initialisers for nulls/values arrays".
That started off with lots of support, but then there was a suggestion
that the {0} should be implemented as a macro, and the subsequent
discussions about that macro eventually bikeshedded the patch to
death.

It might be a good idea if you check that old thread so you can avoid
the same pitfalls. I hope you have more luck than I did ;-)

------
[1]
https://www.postgresql.org/message-id/flat/2793d0d2-c65f-5db0-4f89-251188438391%40gmail.com#102ee1b34a8341f28758efc347874b8a

Kind Regards,
Peter Smith.
Fujitsu Australia

On 01.08.22 19:08, Ranier Vilela wrote:
> Like how 
> https://github.com/postgres/postgres/commit/9fd45870c1436b477264c0c82eb195df52bc0919 
> <https://github.com/postgres/postgres/commit/9fd45870c1436b477264c0c82eb195df52bc0919>
> New attempt to remove more MemSet calls, that are safe.
> 
> Attached v3 patch.

Note that struct initialization does not set padding bits.  So any 
struct that is used as a hash key or that goes to disk or something 
similar needs to be set with memset/MemSet instead.  Various places in 
the code make explicit comments about that, which your patch deletes, 
which is a mistake.  This patch needs to be adjusted carefully with this 
in mind before it can be considered.

On 2022-Aug-11, Ranier Vilela wrote:

> According to:
> https://interrupt.memfault.com/blog/c-struct-padding-initialization

Did you actually read it?

https://interrupt.memfault.com/blog/c-struct-padding-initialization#structure-zero-initialization

: This looks great! However, it’s not obvious (from looking at those snippets)
: what the value loaded into the padding region will be.
:
: The unfortunate answer is: it depends
:
: The C11 standard, chapter §6.2.6.1/6 says this:
:
: : When a value is stored in an object of structure or union type, including in a
: : member object, the bytes of the object representation that correspond to any
: : padding bytes take unspecified values.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)

On Thu, Aug 11, 2022 at 08:51:53AM -0300, Ranier Vilela wrote:
> Em qui., 11 de ago. de 2022 às 08:48, Alvaro Herrera <
> alvherre@alvh.no-ip.org> escreveu:
> 
> > On 2022-Aug-11, Ranier Vilela wrote:
> >
> > > According to:
> > > https://interrupt.memfault.com/blog/c-struct-padding-initialization
> >
> Did you see the Strategy 3 table, { 0 } ?

It explicitly shows that at least Ubuntu clang version 13.0.0-2 with -01
doesn't do anything about the padding bytes (and that's after testing only 2
different compilers).  Even if those compilers didn't show any problem, we
still couldn't rely on an undefined behavior and assume that no other compilers
behave differently.

Hi Ranier,

Following the comment in commit 9fd45870c1436b477264c0c82eb195df52bc0919,

    (The same could be done with appropriate memset() calls, but this
    patch is part of an effort to phase out MemSet(), so it doesn't touch
    memset() calls.)

Should these obviously possible replacement of the standard library function "memset" be considered as well? For example, something like the attached one which is focusing on the pageinspect extension only.

Best regards,

David

Em sáb., 16 de jul. de 2022 às 16:54, Ranier Vilela <ranier.vf@gmail.com> escreveu:

Em sáb, 16 de jul de 2022 2:58 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> escreveu:

On 11.07.22 21:06, Ranier Vilela wrote:
> Em qui., 7 de jul. de 2022 às 14:01, Ranier Vilela <ranier.vf@gmail.com
> <mailto:ranier.vf@gmail.com>> escreveu:
>
>     Attached the v1 of your patch.
>     I think that all is safe to switch MemSet by {0}.
>
> Here the rebased patch v2, against latest head.

I have committed my patch with Álvaro's comments addressed

I see.

It's annoing that old compiler (gcc 4.7.2) don't handle this style.

Your patch appears to add in changes that are either arguably out of
scope or would need further review (e.g., changing memset() calls,
changing the scope of some variables, changing places that need to worry
about padding bits).  Please submit separate patches for those, and we
can continue the analysis.

Sure.

Hi, sorry for the delay.

Like how https://github.com/postgres/postgres/commit/9fd45870c1436b477264c0c82eb195df52bc0919

New attempt to remove more MemSet calls, that are safe.

Attached v3 patch.

regards,

Ranier Vilela

On 2022-Aug-19, David Zhang wrote:

> Should these obviously possible replacement of the standard library function
> "memset" be considered as well? For example, something like the attached one
> which is focusing on the pageinspect extension only.

If you do this, you're creating a potential backpatching hazard.  This
is OK if we get something in return, so a question to ask is whether
there is any benefit in doing it.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Most hackers will be perfectly comfortable conceptualizing users as entropy
 sources, so let's move on."                               (Nathaniel Smith)

On 24.08.22 16:30, Alvaro Herrera wrote:
> On 2022-Aug-19, David Zhang wrote:
>> Should these obviously possible replacement of the standard library function
>> "memset" be considered as well? For example, something like the attached one
>> which is focusing on the pageinspect extension only.
> 
> If you do this, you're creating a potential backpatching hazard.  This
> is OK if we get something in return, so a question to ask is whether
> there is any benefit in doing it.

I don't follow how this is a backpatching hazard.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 24.08.22 16:30, Alvaro Herrera wrote:
>> If you do this, you're creating a potential backpatching hazard.  This
>> is OK if we get something in return, so a question to ask is whether
>> there is any benefit in doing it.

> I don't follow how this is a backpatching hazard.

Call me a trogdolyte, but I don't follow how it's an improvement.
It looks to me like an entirely random change that doesn't get rid
of assumptions about what the bits are, it just replaces one set of
assumptions with a different set.  Moreover, the new set of assumptions
may include "there are no padding bits in here", which is mighty fragile
and hard to verify.  So I frankly do not find this a stylistic improvement.

            regards, tom lane

On Wed, Aug 24, 2022 at 3:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Call me a trogdolyte, but I don't follow how it's an improvement.
> It looks to me like an entirely random change that doesn't get rid
> of assumptions about what the bits are, it just replaces one set of
> assumptions with a different set.  Moreover, the new set of assumptions
> may include "there are no padding bits in here", which is mighty fragile
> and hard to verify.  So I frankly do not find this a stylistic improvement.

Ditto.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Aug 24, 2022 at 3:20 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> But, these same arguments apply to Designated Initializers [1].
>
> like:
> struct foo a = {
>    .i = 0,
>    .b = 0,
> };
>
> That is slowly being introduced and IMHO brings the same problems with padding bits.

Yep. I don't find that an improvement over a MemSet on the struct
either, if we're just using it to fill in zeroes.

If we're using it to fill in non-zero values, though, then there's a
reasonable argument that it offers some notational convenience.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 2022-Aug-24, Peter Eisentraut wrote:

> I don't follow how this is a backpatching hazard.

It changes code.  Any bugfix in the surrounding code would have to fix a
conflict.  That is nonzero effort.  Is it a huge risk?  No, it is very
small risk and a very small cost to fix such a conflict; but my claim is
that this change has zero benefit, therefore we should not incur a
nonzero future effort.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"How amazing is that? I call it a night and come back to find that a bug has
been identified and patched while I sleep."                (Robert Davidson)
               http://archives.postgresql.org/pgsql-sql/2006-03/msg00378.php

On Thu, Aug 25, 2022 at 10:38:41AM +0200, Alvaro Herrera wrote:
> It changes code.  Any bugfix in the surrounding code would have to fix a
> conflict.  That is nonzero effort.  Is it a huge risk?  No, it is very
> small risk and a very small cost to fix such a conflict; but my claim is
> that this change has zero benefit, therefore we should not incur a
> nonzero future effort.

Agreed to leave things as they are.  This really comes down to if we
want to make this code more C99-ish or not, and the post-patch result
is logically the same as the pre-patch result.
--
Michael