Hi,
On 2021-11-02 10:28:39 -0700, Jeff Davis wrote:
> On Mon, 2021-11-01 at 12:50 -0400, Stephen Frost wrote:
> > All that said, I wonder if we can have our cake and eat it too. I
> > haven't looked into this at all yet and perhaps it's foolish on its
> > face, but, could we make CHECKPOINT; basically turn around and just
> > run
> > select pg_checkpoint(); with the regular privilege checking
> > happening?
> > Then we'd keep the existing syntax working, but if the user is
> > allowed
> > to run the command would depend on if they've been GRANT'd EXECUTE
> > rights on the function or not.
>
> Great idea! Patch attached.
>
> This feels like a good pattern that we might want to use elsewhere, if
> the need arises.
> case T_CheckPointStmt:
> - if (!superuser())
> - ereport(ERROR,
> - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> - errmsg("must be superuser to do CHECKPOINT")));
> + {
> + /*
> + * Invoke pg_checkpoint(). Implementing the CHECKPOINT command
> + * with a function allows administrators to grant privileges
> + * on the CHECKPOINT command by granting privileges on the
> + * pg_checkpoint() function. It also calls the function
> + * execute hook, if present.
> + */
> + AclResult aclresult;
> + FmgrInfo flinfo;
> +
> + aclresult = pg_proc_aclcheck(F_PG_CHECKPOINT, GetUserId(),
> + ACL_EXECUTE);
> + if (aclresult != ACLCHECK_OK)
> + aclcheck_error(aclresult, OBJECT_FUNCTION,
> + get_func_name(F_PG_CHECKPOINT));
>
> - RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT |
> - (RecoveryInProgress() ? 0 : CHECKPOINT_FORCE));
> + InvokeFunctionExecuteHook(F_PG_CHECKPOINT);
> +
> + fmgr_info(F_PG_CHECKPOINT, &flinfo);
> +
> + (void) FunctionCall0Coll(&flinfo, InvalidOid);
> + }
> break;
I don't like this. This turns the checkpoint command which previously didn't
rely on the catalog in the happy path etc into something that requires most of
the backend to be happily up to work.
Greetings,
Andres Freund