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

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.