Horton

Thread

Commits

  1. ecpg: invent a saner syntax for ecpg.addons entries.

  2. ecpg: add cross-checks to parse.pl for usage of internal tables.

  1. Improving the notation for ecpg.addons rules

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-18T17:13:36Z

    I've gotten annoyed by the notation used for ecpg.addons rules,
in which all the tokens of the gram.y rule to be modified
are just concatenated.  This is unreadable and potentially
ambiguous:

ECPG: fetch_argsABSOLUTE_PSignedIconstopt_from_incursor_name addon

The attached draft patch changes things so that we can write

ECPG: addon fetch_args ABSOLUTE_P SignedIconst opt_from_in cursor_name

which is a whole lot closer to what actually appears in gram.y:

fetch_args:    cursor_name
            ...
            | ABSOLUTE_P SignedIconst opt_from_in cursor_name

(Note that I've also moved the rule type "addon" to the front.
This isn't strictly necessary, but it seems less mistake-prone.)

While I've not done it in the attached, perhaps it would be
an improvement to allow a colon after the target nonterminal:

ECPG: addon fetch_args: ABSOLUTE_P SignedIconst opt_from_in cursor_name

to bring it even closer to what is written in gram.y.  You could
imagine going further and writing this case as something like

ECPG: addon fetch_args | ABSOLUTE_P SignedIconst opt_from_in cursor_name

but I think that might be a step too far.  IMO it's not adding much
readability, and it seems like introducing an unnecessary dependency
on exactly how the gram.y alternatives are laid out.

BTW, the attached patch won't apply to HEAD, it's meant to apply
after the patch series being discussed at [1].  So I won't stick
this in the CF yet.

Thoughts?

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/2011420.1713493114@sss.pgh.pa.us

  2. Re: Improving the notation for ecpg.addons rules

    Michael Paquier <michael@paquier.xyz> — 2024-08-19T05:17:34Z

    On Sun, Aug 18, 2024 at 01:13:36PM -0400, Tom Lane wrote:
> While I've not done it in the attached, perhaps it would be
> but I think that might be a step too far.  IMO it's not adding much
> readability, and it seems like introducing an unnecessary dependency
> on exactly how the gram.y alternatives are laid out.

Not being too aggressive with the changes sounds like a good thing
here.

> BTW, the attached patch won't apply to HEAD, it's meant to apply
> after the patch series being discussed at [1].  So I won't stick
> this in the CF yet.
> 
> Thoughts?

Seeing changes like "stmtClosePortalStmt" changing to "stmt
ClosePortalStmt" is clearly an improvement in readability.
SignedIconstIconst was also fun.  Your change is a good idea.

It looks like %replace_line expects all its elements to have one space
between each token, still this is not enforced with a check across its
hardcoded elements?
--
Michael

  3. Re: Improving the notation for ecpg.addons rules

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-19T05:23:58Z

    Michael Paquier <michael@paquier.xyz> writes:
> It looks like %replace_line expects all its elements to have one space
> between each token, still this is not enforced with a check across its
> hardcoded elements?

Yeah, I was wondering about that.  I wouldn't do it exactly like
that, but with a check that the entry gets matched somewhere.
The other patchset adds cross-checks that each ecpg.addons entry is
used exactly once, but there's no such check for these arrays that
are internal to parse.pl.  Can't quite decide if it's worth adding.

I debugged the patch in this thread by checking that the emitted
preproc.y didn't change, but that might not be helpful for changes
that are actually intended to change the grammar.

			regards, tom lane

  4. Re: Improving the notation for ecpg.addons rules

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-20T18:33:23Z

    I wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> It looks like %replace_line expects all its elements to have one space
>> between each token, still this is not enforced with a check across its
>> hardcoded elements?

> Yeah, I was wondering about that.  I wouldn't do it exactly like
> that, but with a check that the entry gets matched somewhere.

Here's a patch for that (again based on the other patch series).
This did not turn up anything interesting, but it's probably
worth keeping.

			regards, tom lane

  5. Re: Improving the notation for ecpg.addons rules

    Michael Paquier <michael@paquier.xyz> — 2024-08-23T00:22:22Z

    On Tue, Aug 20, 2024 at 02:33:23PM -0400, Tom Lane wrote:
> I wrote:
>> Yeah, I was wondering about that.  I wouldn't do it exactly like
>> that, but with a check that the entry gets matched somewhere.
> 
> Here's a patch for that (again based on the other patch series).
> This did not turn up anything interesting, but it's probably
> worth keeping.

Okay, I see where you're going with this one.  It does not seem like
this is going to cost much in long-term maintenance while catching
unfortunate issues, so +1 from me.

The patch does not apply on HEAD due to the dependency with the other
things you are proposing, and I would have hardcoded failures to check
that the reports are correct, but that looks neat on read.
--
Michael

  6. Re: Improving the notation for ecpg.addons rules

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-23T02:03:37Z

    Michael Paquier <michael@paquier.xyz> writes:
> The patch does not apply on HEAD due to the dependency with the other
> things you are proposing, and I would have hardcoded failures to check
> that the reports are correct, but that looks neat on read.

I did test it by injecting errors, but I don't see why we'd leave
those in place?

Anyway, my thought was to merge this into the other patch series,
but I didn't do that yet.

			regards, tom lane

  7. Re: Improving the notation for ecpg.addons rules

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-14T20:16:54Z

    I wrote:
> Anyway, my thought was to merge this into the other patch series,
> but I didn't do that yet.

I'd forgotten about merging these two patches into the bigger
ecpg patch series.  Since there didn't seem to be any objection
to either, I went ahead and pushed them.

			regards, tom lane