Thread: Re: Pass ParseState as down to utility functions.
On 2024-Dec-06, jian he wrote:
> From 6bf657c3b62b7460b317c42ce2f4fa0988acf1a0 Mon Sep 17 00:00:00 2001
> From: jian he <jian.universality@gmail.com>
> Date: Fri, 6 Dec 2024 16:37:18 +0800
> Subject: [PATCH v6 1/1] print out error position for some DDL command
>
> doing this by passing the source_string to the existing ParseState
> or by making a new ParseState passing source_string to it.
>
> With this patch, the following functions will printout the error position for certain error cases.
>
> ATExecAddOf
> DefineType
> ATPrepAlterColumnType
> ATExecAlterColumnType
> DefineDomain
> AlterType
> transformAlterTableStmt
I think it would make more sense to write the commit message in terms of
the DDL commands that now report error position, than the C functions.
Such a list of commands does not need to be exhaustive; a
representative-enough sample probably suffices.
> @@ -943,11 +942,13 @@ DefineDomain(CreateDomainStmt *stmt)
> if (constr->is_no_inherit)
> ereport(ERROR,
> - errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> - errmsg("not-null constraints for domains cannot be marked NO INHERIT"));
> + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> + errmsg("not-null constraints for domains cannot be marked NO INHERIT"),
> + parser_errposition(pstate, constr->location)));
Once upon a time, ereport() was a simpler macro that did not
use variadic arguments. Back then, the list of functions embedded in it
(errcode, errmsg etc) were forced to be in an additional level of
parentheses so that the macro would work at all (IIRC failure to do that
resulted in strange compile-time problems). This is why a majority of
code is written in the style with those parens. But commit e3a87b4991cc
changed ereport to use __VA_ARGS__, so the auxiliary functions are
actual arguments to errstart() -- which means that the parentheses
you're adding here are unnecessary and discouraged. Just add the
parser_errposition() call and it'll be fine.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"XML!" Exclaimed C++. "What are you doing here? You're not a programming
language."
"Tell that to the people who use me," said XML.
https://burningbird.net/the-parable-of-the-languages/
jian he <jian.universality@gmail.com> writes:
> add parser_errposition to some places in
> transformTableConstraint, transformColumnDefinition
> where v8 didn't.
I'm not loving the idea of cons'ing up ParseStates in random places in
tablecmds.c. I think we ought to fix things so that the one made in
standard_ProcessUtility is passed down to all these places, replacing
ad-hoc queryString and queryEnv parameters.
Eventually we might want to make ProcessUtility's callers pass down
a pstate, but that would be a whole other area of invasive changes,
so I think we should leave that idea for later. Right now though,
it seems reasonable to change AlterTableUtilityContext to replace
the queryString and queryEnv fields with a ParseState carrying that
info --- and, perhaps, someday saving us from adding more ad-hoc
fields there.
regards, tom lane
On Thu, Dec 12, 2024 at 4:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > jian he <jian.universality@gmail.com> writes: > > add parser_errposition to some places in > > transformTableConstraint, transformColumnDefinition > > where v8 didn't. > > I'm not loving the idea of cons'ing up ParseStates in random places in > tablecmds.c. I think we ought to fix things so that the one made in > standard_ProcessUtility is passed down to all these places, replacing > ad-hoc queryString and queryEnv parameters. > the main code change is within DefineDomain. AlterTableUtilityContext comments says: /* Info needed when recursing from ALTER TABLE */ so we cannot pass DefineDomain with AlterTableUtilityContext. -DefineDomain(CreateDomainStmt *stmt) +DefineDomain(ParseState *pstate, CreateDomainStmt *stmt) we have to pass either ParseState or queryString to DefineDomain. -extern ObjectAddress AlterType(AlterTypeStmt *stmt); +extern ObjectAddress AlterType(ParseState *pstate, AlterTypeStmt *stmt); this change not necessary, we can remove it. but other places (listed in below), we are passing (AlterTableUtilityContext *context) which seems ok? -ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode) +ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode, + AlterTableUtilityContext *context) static ObjectAddress ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, - AlterTableCmd *cmd, LOCKMODE lockmode); + AlterTableCmd *cmd, LOCKMODE lockmode, + AlterTableUtilityContext *context); static ObjectAddress ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, - AlterTableCmd *cmd, LOCKMODE lockmode) + AlterTableCmd *cmd, LOCKMODE lockmode, + AlterTableUtilityContext *context)