Re: Better error reporting from extension scripts (Was: Extend ALTER OPERATOR)

po 14. 10. 2024 v 5:38 odesílatel jian he <jian.universality@gmail.com>
napsal:

> On Mon, Oct 14, 2024 at 1:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Pavel Stehule <pavel.stehule@gmail.com> writes:
> > > so 12. 10. 2024 v 9:33 odesílatel jian he <jian.universality@gmail.com
> >
> > > napsal:
> > >> + /*
> > >> + * If we have a location (which, as said above, we really always
> should)
> > >> + * then report a line number to aid in localizing problems in big
> scripts.
> > >> + */
> > >> + if (location >= 0)
> > >> so this part will always be true?
> >
> > > yes, after  CleanQuerytext the location should not be -1 ever
> >
> > Right, but we might not have entered either of those previous
> > if-blocks.
>
>
> in src/backend/parser/gram.y
> your makeRawStmt changes (v4) seem to guarantee that
> RawStmt->stmt_location >= 0.
> other places {DefineView,DoCopy,PrepareQuery} use makeNode(RawStmt),
> In these cases, I am not so sure RawStmt->stmt_location >=0  is always
> true.
>
> in execute_sql_string
>
>     raw_parsetree_list = pg_parse_query(sql);
>    dest = CreateDestReceiver(DestNone);
>     foreach(lc1, raw_parsetree_list)
>     {
>         RawStmt    *parsetree = lfirst_node(RawStmt, lc1);
>         MemoryContext per_parsetree_context,
>                     oldcontext;
>         List       *stmt_list;
>         ListCell   *lc2;
>         callback_arg.stmt_location = parsetree->stmt_location;
>         callback_arg.stmt_len = parsetree->stmt_len;
>         per_parsetree_context =
>             AllocSetContextCreate(CurrentMemoryContext,
>                                   "execute_sql_string per-statement
> context",
>                                   ALLOCSET_DEFAULT_SIZES);
>         oldcontext = MemoryContextSwitchTo(per_parsetree_context);
>         CommandCounterIncrement();
>         stmt_list = pg_analyze_and_rewrite_fixedparams(parsetree,
>                                                        sql,
>                                                        NULL,
>                                                        0,
>                                                        NULL);
>
> Based on the above code, we do
> `callback_arg.stmt_location = parsetree->stmt_location;`
> pg_parse_query(sql) doesn't use script_error_callback.
>
> So if we are in script_error_callback
> `int            location = callback_arg->stmt_location;`
> location >= 0 will be always true?
>
>
>
> > The question here is whether the raw parser (gram.y)
> > ever throws an error that doesn't include a cursor position.  IMO it
> > shouldn't, but a quick look through gram.y finds a few ereports that
> > lack parser_errposition.  We could go fix those, and probably should,
> > but imagining that none will ever be introduced again seems like
> > folly.
> >
>
> I don't know how to add the error position inside the function
> insertSelectOptions.
> maybe we can add
> `parser_errposition(exprLocation(limitClause->limitCount))));`
> but limitCount position is a nearby position.
> I am also not sure about func mergeTableFuncParameters.
>
>
> for other places in gram.y, I've added error positions for ereport
> that lack it  , please check the attached.
>

I think this can be a separate commitfest issue.

Regards

Pavel