Hi,
On Thu, Sep 18, 2025 at 11:52:05AM -0500, Sami Imseih wrote:
> > I still want to add it, but it also seemed like you were not much a
> > fan of it, so I did not really want to push forward with something
> > that was not loved. :D
> >
> > Addressing your points, attached is an updated patch l
> More suggestions or a better sentence are of course welcome.
> "NB: We assume that this struct contains no padding. The 8 bytes
> allocated for the object ID are good enough to ensure the uniqueness
> of the hash key, hence the addition of new fields is not recommended."
That sounds correct. H
>
> Exactly. Hence my proposal of static assertion is doing exactly the
> job I want it to do :)
But my concern is the flexibility of this approach. If someone is to add an
OID field next, they will not be able to as that will be introducing
padding.
On the other hand, passing the key by referen
Em qua., 10 de set. de 2025 às 23:53, Michael Paquier
escreveu:
> On Mon, Sep 08, 2025 at 09:36:52PM -0500, Sami Imseih wrote:
> > But my concern is the flexibility of this approach. If someone is to add
> an
> > OID field next, they will not be able to as that will be introducing
> > padding. O
On Fri, Sep 19, 2025 at 08:52:24PM -0500, Sami Imseih wrote:
> Thank you! I update the CF entry [0] as committed.
>
> [0] https://commitfest.postgresql.org/patch/6033/
Oops, missed this part. Thanks.
--
Michael
signature.asc
Description: PGP signature
> On Thu, Sep 18, 2025 at 11:52:05AM -0500, Sami Imseih wrote:
> > v2 looks good to me. Not sure if there are edge cases in which
> > the assert will succeed even with padding (I can't think of any way
> > that will be possible), so for now what you have seems good enough.
>
> Okay, I've applied t
On Thu, Sep 18, 2025 at 11:52:05AM -0500, Sami Imseih wrote:
> v2 looks good to me. Not sure if there are edge cases in which
> the assert will succeed even with padding (I can't think of any way
> that will be possible), so for now what you have seems good enough.
Okay, I've applied that on HEAD
> I still want to add it, but it also seemed like you were not much a
> fan of it, so I did not really want to push forward with something
> that was not loved. :D
>
> Addressing your points, attached is an updated patch labelled v2.
I was not a fan of it, when my idea was to allow flexibility in
On Wed, Sep 17, 2025 at 07:04:36PM -0500, Sami Imseih wrote:
> "NB: We assume that this struct contains no padding. Also, 8 bytes
> allocated for the object ID are good enough to ensure the uniqueness
> of the hash key, hence the addition of new fields is not recommended."
Works for me.
> Also,
On Tue, Sep 16, 2025 at 02:38:20PM -0500, Sami Imseih wrote:
> 7d85d87f4d5c35 added code to clear the padding bytes with memset
> in anticipation that the key could be changed in the future, in a way
> that padding will be introduced.
Yep. The argument raised on this thread with the requirement o
> On Mon, Sep 15, 2025 at 04:47:27PM -0500, Sami Imseih wrote:
> > Just to confirm, you are saying we are unlikely to ever add a new field
> > to the key. Is that correct?
>
> I would rather avoid that, yes.
7d85d87f4d5c35 added code to clear the padding bytes with memset
in anticipation that the
> On Mon, Sep 08, 2025 at 09:36:52PM -0500, Sami Imseih wrote:
> > But my concern is the flexibility of this approach. If someone is to add an
> > OID field next, they will not be able to as that will be introducing
> > padding. On the other hand, passing the key by reference and
> > documenting t
On Mon, Sep 15, 2025 at 04:47:27PM -0500, Sami Imseih wrote:
> Just to confirm, you are saying we are unlikely to ever add a new field
> to the key. Is that correct?
I would rather avoid that, yes.
--
Michael
signature.asc
Description: PGP signature
> > I don't see how this improves the situation, but will just make it more
> > difficult to add a new field that requires padding in the future.
> >
> > If we are documenting either way, I rather that we just document the need
> > to pass a key by reference, which is the pattern used in other area
On Thu, Sep 11, 2025 at 10:21:45AM -0500, Sami Imseih wrote:
> I don't see how this improves the situation, but will just make it more
> difficult to add a new field that requires padding in the future.
>
> If we are documenting either way, I rather that we just document the need
> to pass a key b
On Mon, Sep 08, 2025 at 09:36:52PM -0500, Sami Imseih wrote:
> But my concern is the flexibility of this approach. If someone is to add an
> OID field next, they will not be able to as that will be introducing
> padding. On the other hand, passing the key by reference and
> documenting the reason
On Mon, Sep 08, 2025 at 09:20:14PM -0500, Sami Imseih wrote:
> The following will fail the assert since padding is needed for the
> new Oid member.
>
> @@ -53,6 +53,7 @@ typedef struct PgStat_HashKey
> {
> PgStat_Kind kind; /* statistics entry kind */
> Oid
> > I'd just add a comment mentioning that any padding bytes should be avoided.
>
> Agreed on this part.
>
> I am wondering also about the addition of a static assertion as well,
> as it seems that this thread and the opinions on it point to such an
> addition having value, and requiring valgrind t
On Mon, Sep 08, 2025 at 12:08:12PM -0400, Andres Freund wrote:
> On 2025-09-08 10:25:13 +0900, Michael Paquier wrote:
>> Another idea would be to make sure that the sizeof() of the structure
>> matches with the sum of the sizeof() for the individual fields in it.
>> That's cumbersome to rely on, st
> Did you run a full installcheck under valgrind to have more confidence
> that this is the only code path where we care about the padding of the
> passed-in values for the hash lookups?
I did and could not reproduce "Use of uninitialised value..." on HEAD.
I will mention that it's not a simple r
Hi,
On 2025-09-08 10:25:13 +0900, Michael Paquier wrote:
> On Sat, Sep 06, 2025 at 10:35:58AM +0900, Michael Paquier wrote:
> > One idea would be, for example, that keys used with simplehash should
> > never have any padding at all, and we could force a check in the shape
> > of a static assertion
On Sat, Sep 06, 2025 at 10:35:58AM +0900, Michael Paquier wrote:
> One idea would be, for example, that keys used with simplehash should
> never have any padding at all, and we could force a check in the shape
> of a static assertion to force a failure when attempting to compile
> code that attempt
On Thu, Sep 04, 2025 at 11:50:15AM -0500, Sami Imseih wrote:
> Perhaps calling this a compiler bug is not appropriate.
> However, memset is in fact called when the key is created
>
> ```
> /* clear padding */
> memset(&key, 0, sizeof(struct PgStat_HashKey));
> ```
>
> but the zeroed out paddi
> +1
> Not to mention that it is faster to pass the parameter by reference.
yes, there is potentially better performance.
BTW. My mistake. The subject of this thread should be
"PgStat_HashKey padding issue when passed by value"
--
Sami Imseih
Amazon Web Services (AWS)
Em qui., 4 de set. de 2025 às 13:15, Sami Imseih
escreveu:
> Hi,
>
> I was experimenting with adding a new OID field to PgStat_HashKey:
>
> ```
> typedef struct PgStat_HashKey
> {
> PgStat_Kind kind; /* statistics entry kind */
> Oid dboid; /* database ID. InvalidOid for shared o
Hi,
On 2025-09-04 11:14:53 -0500, Sami Imseih wrote:
> GDB when setting a breakpoint at pgstat_entry_ref_hash_insert (which
> is called inside
> pgstat_get_entry_ref_cached) shows that the padding has some unexpected
> values, see the 12-15th byte "0x00 0x23 0x21 0x21" which based on the
> memset
> > This looks like a compiler bug, we tested multiple ways to workaround:
>
> Padding bytes aren't guaranteed to be zeroed, unless you take care to zero
> them out with memset or such. That's not a compiler bug.
Perhaps calling this a compiler bug is not appropriate.
However, memset is in fact ca
I forgot to CC Bertrand as mentioned above.
--
Sami Imseih
Amazon Web Services (AWS)
28 matches
Mail list logo
