Re: PgStat_HashKey padding issue when passed by reference - Mailing list pgsql-hackers

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 objects. */
  Oid     newfield;
  uint64   objid;     /* object ID (table, function, etc.), or
                 * identifier. */
} PgStat_HashKey;
```

It's important to observe that hole padding is added with this new
field:

```
(gdb) ptype /o PgStat_HashKey
type = struct PgStat_HashKey {
/*   0   |    4 */  uint32 kind;
/*   4   |    4 */  Oid dboid;
/*   8   |    4 */  Oid newfield;
/* XXX 4-byte hole   */
/*   16   |    8 */  uint64 objid;

                /* total size (bytes):  24 */
               }
```

With "newfield" added, I started seeing the error "entry ref vanished
before deletion" when creating the cluster (or selecting from
pg_stat_database, etc.).

This error occurs when the local reference to a pgstat entry is deleted
and the entry can't be found in the simplehash pgStatEntryRefHash,
based on the key.

```
  if (!pgstat_entry_ref_hash_delete(pgStatEntryRefHash, key))
    elog(ERROR, "entry ref vanished before deletion");
```

This is surprising because this entry with the key is created earlier
via pgstat_get_entry_ref_cached -> pgstat_entry_ref_hash_insert.

When I brought this up to Bertrand (CC'd), he was not able to reproduce the
ERROR by adding a new field, and we quickly realized that it is related
to the compiler optimization (-O) flags we were using. He was building
with -O0 (no optimization) and I was building with -O2. So something
with optimization was causing the issue.

Starting a problematic cluster with Valgrind:

```
valgrind --leak-check=full --track-origins=yes \
  $PGHOME/bin/postgres -D $PGDATA --single -P
```

shows messages like "Use of uninitialised value of size 8":

```
==26625== Use of uninitialised value of size 8
==26625==  at 0x61D04F: pgstat_get_entry_ref_cached (pgstat_shmem.c:405)
==26625==  by 0x61D04F: pgstat_get_entry_ref (pgstat_shmem.c:488)
==26625==  by 0x6177B9: pgstat_fetch_entry (pgstat.c:977)
==26625==  by 0x5433B2: rebuild_database_list (autovacuum.c:1002)
```

Further debugging led us to the fact that the key is passed-by-value to

```
pgstat_get_entry_ref_cached(PgStat_HashKey key, PgStat_EntryRef **entry_ref_p)
```

and in that routine the entry is created:

```
cache_entry = pgstat_entry_ref_hash_insert(pgStatEntryRefHash, key, &found);
``

The hash for the key is created with fasthash32(key, size, 0) and
compared against another key using memcmp(a, b, sizeof(PgStat_HashKey))
in the "pgstat_hash_hash_key" and "pgstat_cmp_hash_key" routines,
respectively.

With -O2, we observed that (on our testing environment):

pgstat_get_entry_ref_cached() is inlined
the inlined code does not copy the hole padding content when the key
is passed-by-value

So that while the hole initially contains zeroes:

```
  /* clear padding */
  memset(&key, 0, sizeof(struct PgStat_HashKey));
```

We observed that the zeroes in the hole are not copied to
pgstat_get_entry_ref_cached()
when passed-by-value with -O2.

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 done earlier should be 0.

```
(gdb) x/24xb d
0x7fff68cc16f0:  0x02  0x00  0x00  0x00  0x05  0x00  0x00  0x00
0x7fff68cc16f8:  0x00  0x00  0x00  0x00  0x00  0x23  0x21  0x21
0x7fff68cc1700:  0x67  0x0a  0x00  0x00  0x00  0x00  0x00  0x00
(gdb)
```

Now, if we pass the key by reference to pgstat_entry_ref_hash_insert as is
patched, we can see the padding is in fact cleared.

```
(gdb) x/24xb d
0x7ffe1bf6c640:  0x01  0x00  0x00  0x00  0x05  0x00  0x00  0x00
0x7ffe1bf6c648:  0x00  0x00  0x00  0x00  0x00  0x00  0x00  0x00
0x7ffe1bf6c650:  0x00  0x00  0x00  0x00  0x00  0x00  0x00  0x00
```

This looks like a compiler bug, we tested multiple ways to workaround:

1/ pg_noinline pgstat_get_entry_ref_cached
2/ making volatile the PgStat_HashKey key in the
pgstat_get_entry_ref_cached parameter
3/ Passing the key by reference instead of by value
4/ set SH_SCOPE to "external" rather than "static inline" in pgstat_shmem.c

it seems, at least for this case, the best option is to pass the key
by reference, like
in the attached patch.