Re: Statistics updates is delayed when using `commit and chain`

Hi,

On 2022-05-14 17:11:33 +0200, Laetitia Avrot wrote:
> This patch includes the update of the statistics in case of commit and
> chain.

Hm.

> @@ -3006,6 +3006,12 @@ CommitTransactionCommand(void)
>              s->blockState = TBLOCK_DEFAULT;
>              if (s->chain)
>              {
> +                /*
> +                 * Before starting the new transaction, we'll update the
> +                 * statistics so that autovacuum can be triggered without
> +                 * waiting for a `commit` or `rollback` without `and chain`.
> +                 */
> +                pgstat_report_stat(false);
>                  StartTransaction();
>                  s->blockState = TBLOCK_INPROGRESS;
>                  s->chain = false;
> @@ -3032,6 +3038,12 @@ CommitTransactionCommand(void)
>              s->blockState = TBLOCK_DEFAULT;
>              if (s->chain)
>              {
> +                /*
> +                 * Before starting the new transaction, we'll update the
> +                 * statistics so that autovacuum can be triggered without
> +                 * waiting for a `commit` or `rollback` without `and chain`.
> +                 */
> +                pgstat_report_stat(false);
>                  StartTransaction();
>                  s->blockState = TBLOCK_INPROGRESS;
>                  s->chain = false;

> @@ -3050,6 +3062,12 @@ CommitTransactionCommand(void)
>              s->blockState = TBLOCK_DEFAULT;
>              if (s->chain)
>              {
> +                /*
> +                 * Before starting the new transaction, we'll update the
> +                 * statistics so that autovacuum can be triggered without
> +                 * waiting for a `commit` or `rollback` without `and chain`.
> +                 */
> +                pgstat_report_stat(false);
>                  StartTransaction();
>                  s->blockState = TBLOCK_INPROGRESS;
>                  s->chain = false;
> @@ -3117,6 +3135,12 @@ CommitTransactionCommand(void)
>                  s->blockState = TBLOCK_DEFAULT;
>                  if (s->chain)
>                  {
> +                    /*
> +                     * Before starting the new transaction, we'll update the
> +                     * statistics so that autovacuum can be triggered without
> +                     * waiting for a `commit` or `rollback` without `and chain`.
> +                     */
> +                    pgstat_report_stat(false);
>                      StartTransaction();
>                      s->blockState = TBLOCK_INPROGRESS;
>                      s->chain = false;

I don't like copying the same comment etc into multiple places. Given the
number of identical blocks for if (s->chain) blocks, perhaps we should just
move it into a new helper?

I'm a bit worried about triggering pgstat_report_stat() in new places where it
previously couldn't be called. There's a few places in the relation stats code
(predating the shared memory stats patch) that don't cope well with being
called in the wrong state (see e.g. 0cf16cb8ca4), and I'm not sure how careful
the procedures patch was with ensuring that there aren't any cases of
relations being kept open across transactions or something like that.

Greetings,

Andres Freund