Horton

Thread

Commits

  1. Strip Windows newlines from extension script files manually.

  2. Read extension script files in text not binary mode.

  3. Improve reporting of errors in extension script files.

  4. Improve parser's reporting of statement start locations.

  5. Extend ALTER OPERATOR to allow setting more optimization attributes.

  6. Core support for "extensions", which are packages of SQL objects.

  1. [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tommy Pavlicek <tommypav122@gmail.com> — 2023-06-22T16:35:10Z

    Hi All,

I've attached a couple of patches to allow ALTER OPERATOR to add
commutators, negators, hashes and merges to operators that lack them.

The need for this arose adding hash functions to the ltree type after the
operator had been created without hash support[1]. There are potential
issues with modifying these attributes that have been discussed
previously[2], but I understand that setting them, if they have not been
set before, is ok.

I belatedly realised that it may not be desirable or necessary to allow
adding commutators and negators in ALTER OPERATOR because the linkage can
already be added when creating a new operator. I don't know what's best, so
I thought I'd post this here and get feedback before removing anything.

The first patch is create_op_fixes_v1.patch and it includes some
refactoring in preparation for the ALTER OPERATOR changes and fixes a
couple of minor bugs in CREATE OPERATOR:
- prevents self negation when filling in/creating an existing shell operator
- remove reference to sort operator in the self negation error message as
the sort attribute appears to be deprecated in Postgres 8.3

The second patch is alter_op_v1.patch which contains the changes to ALTER
OPERATOR and depends on create_op_fixes_v1.patch.

Additionally, I wasn't sure whether it was preferred to fail or succeed on
ALTERs that have no effect, such as adding hashes on an operator that
already allows them or disabling hashes on one that does not. I chose to
raise an error when this happens, on the thinking it was more explicit and
made the code simpler, even though the end result would be what the user
wanted.

Comments appreciated.

Thanks,
Tommy

[1]
https://www.postgresql.org/message-id/flat/CAEhP-W9ZEoHeaP_nKnPCVd_o1c3BAUvq1gWHrq8EbkNRiS9CvQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/flat/3348985.V7xMLFDaJO@dinodell

  2. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-06-22T16:47:48Z

    Tommy Pavlicek <tommypav122@gmail.com> writes:
> I've attached a couple of patches to allow ALTER OPERATOR to add
> commutators, negators, hashes and merges to operators that lack them.

Please add this to the upcoming commitfest [1], to ensure we don't
lose track of it.

> The first patch is create_op_fixes_v1.patch and it includes some
> refactoring in preparation for the ALTER OPERATOR changes and fixes a
> couple of minor bugs in CREATE OPERATOR:
> - prevents self negation when filling in/creating an existing shell operator
> - remove reference to sort operator in the self negation error message as
> the sort attribute appears to be deprecated in Postgres 8.3

Hmm, yeah, I bet nobody has looked at those edge cases in awhile.

> Additionally, I wasn't sure whether it was preferred to fail or succeed on
> ALTERs that have no effect, such as adding hashes on an operator that
> already allows them or disabling hashes on one that does not. I chose to
> raise an error when this happens, on the thinking it was more explicit and
> made the code simpler, even though the end result would be what the user
> wanted.

You could argue that both ways I guess.  We definitely need to raise error
if the command tries to change an existing nondefault setting, since that
might break things as per previous discussion.  But perhaps rejecting
an attempt to set the existing setting is overly nanny-ish.  Personally
I think I'd lean to "don't throw an error if we don't have to", but I'm
not strongly set on that position.

(Don't we have existing precedents that apply here?  I can't offhand
think of any existing ALTER commands that would reject no-op requests,
but maybe that's not a direct precedent.)

			regards, tom lane

[1] https://commitfest.postgresql.org/43/

  3. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2023-06-22T16:54:54Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:

> Tommy Pavlicek <tommypav122@gmail.com> writes:
>
>> Additionally, I wasn't sure whether it was preferred to fail or succeed on
>> ALTERs that have no effect, such as adding hashes on an operator that
>> already allows them or disabling hashes on one that does not. I chose to
>> raise an error when this happens, on the thinking it was more explicit and
>> made the code simpler, even though the end result would be what the user
>> wanted.
>
> You could argue that both ways I guess.  We definitely need to raise error
> if the command tries to change an existing nondefault setting, since that
> might break things as per previous discussion.  But perhaps rejecting
> an attempt to set the existing setting is overly nanny-ish.  Personally
> I think I'd lean to "don't throw an error if we don't have to", but I'm
> not strongly set on that position.
>
> (Don't we have existing precedents that apply here?  I can't offhand
> think of any existing ALTER commands that would reject no-op requests,
> but maybe that's not a direct precedent.)

Since it only supports adding these operations if they don't already
exist, should it not be ALTER OPERATOR ADD <thing>, not SET <thing>?

That makes it natural to add an IF NOT EXISTS clause, like ALTER TABLE
ADD COLUMN has, to make it a no-op instead of an error.

> 			regards, tom lane

- ilmari

  4. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-06-22T17:36:14Z

    =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> (Don't we have existing precedents that apply here?  I can't offhand
>> think of any existing ALTER commands that would reject no-op requests,
>> but maybe that's not a direct precedent.)

> Since it only supports adding these operations if they don't already
> exist, should it not be ALTER OPERATOR ADD <thing>, not SET <thing>?
> That makes it natural to add an IF NOT EXISTS clause, like ALTER TABLE
> ADD COLUMN has, to make it a no-op instead of an error.

Hmm, maybe.  But it feels like choosing syntax and semantics based
on what might be only a temporary implementation restriction.  We
certainly don't handle any other property-setting commands that way.

Admittedly, "can't change an existing setting" is likely to be pretty
permanent in this case, just because I don't see a use-case for it
that'd justify the work involved.  (My wife recently gave me a coffee
cup that says "Nothing is as permanent as a temporary fix.")  But
still, if someone did show up and do that work, we'd regret this
choice of syntax because it'd then be uselessly unlike every other
ALTER command.

			regards, tom lane

  5. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tommy Pavlicek <tommypav122@gmail.com> — 2023-06-23T10:34:38Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:

> Please add this to the upcoming commitfest [1], to ensure we don't
> lose track of it.

I've added a single patch here: https://commitfest.postgresql.org/43/4389/

It wasn't obvious whether I should create a second commitfest entry
because I've included 2 patches so I've just done 1 to begin with. On
that note, is it preferred here to split patches of this size into
separate patches, and if so, additionally, separate threads?

Tom Lane <tgl@sss.pgh.pa.us> writes:

> > Additionally, I wasn't sure whether it was preferred to fail or succeed on
> > ALTERs that have no effect, such as adding hashes on an operator that
> > already allows them or disabling hashes on one that does not. I chose to
> > raise an error when this happens, on the thinking it was more explicit and
> > made the code simpler, even though the end result would be what the user
> > wanted.
>
> You could argue that both ways I guess.  We definitely need to raise error
> if the command tries to change an existing nondefault setting, since that
> might break things as per previous discussion.  But perhaps rejecting
> an attempt to set the existing setting is overly nanny-ish.  Personally
> I think I'd lean to "don't throw an error if we don't have to", but I'm
> not strongly set on that position.
>
> (Don't we have existing precedents that apply here?  I can't offhand
> think of any existing ALTER commands that would reject no-op requests,
> but maybe that's not a direct precedent.)

My initial thinking behind the error for a no-op was largely driven by
the existence of 'DROP.. IF EXISTS'. However, I did some ad hoc
testing on ALTER commands and it does seem that they mostly allow
no-ops. I did find that renaming an object to the same name will fail
due to the object already existing, but that seems to be more of a
coincidence than a design decision to me. Given this, I also lean
towards allowing the no-ops and will change it unless there are
objections.

  6. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-06-23T11:21:46Z

    Tommy Pavlicek <tommypav122@gmail.com> writes:
> I've added a single patch here: https://commitfest.postgresql.org/43/4389/

> It wasn't obvious whether I should create a second commitfest entry
> because I've included 2 patches so I've just done 1 to begin with. On
> that note, is it preferred here to split patches of this size into
> separate patches, and if so, additionally, separate threads?

No, our commitfest infrastructure is unable to deal with patches that have
interdependencies unless they're presented in a single email.  So just use
one thread, and be sure to attach all the patches each time.

(BTW, while you seem to have gotten away with it so far, it's usually
advisable to name the patch files like 0001-foo.patch, 0002-bar.patch,
etc, to make sure the cfbot understands what order to apply them in.)

			regards, tom lane

  7. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tommy Pavlicek <tommypav122@gmail.com> — 2023-07-02T14:42:53Z

    On Fri, Jun 23, 2023 at 12:21 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Tommy Pavlicek <tommypav122@gmail.com> writes:
> > I've added a single patch here: https://commitfest.postgresql.org/43/4389/
>
> > It wasn't obvious whether I should create a second commitfest entry
> > because I've included 2 patches so I've just done 1 to begin with. On
> > that note, is it preferred here to split patches of this size into
> > separate patches, and if so, additionally, separate threads?
>
> No, our commitfest infrastructure is unable to deal with patches that have
> interdependencies unless they're presented in a single email.  So just use
> one thread, and be sure to attach all the patches each time.
>
> (BTW, while you seem to have gotten away with it so far, it's usually
> advisable to name the patch files like 0001-foo.patch, 0002-bar.patch,
> etc, to make sure the cfbot understands what order to apply them in.)
>
>                         regards, tom lane

Thanks.

I've attached a new version of the ALTER OPERATOR patch that allows
no-ops. It should be ready to review now.

  8. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    jian he <jian.universality@gmail.com> — 2023-09-25T10:52:03Z

    /*
 * AlterOperator
 * routine implementing ALTER OPERATOR <operator> SET (option = ...).
 *
 * Currently, only RESTRICT and JOIN estimator functions can be changed.
 */
ObjectAddress
AlterOperator(AlterOperatorStmt *stmt)

The above comment needs to change, other than that, it passed the
test, works as expected.

  9. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    jian he <jian.universality@gmail.com> — 2023-09-25T14:55:01Z

    in doc/src/sgml/ref/alter_operator.sgml

   <varlistentry>
    <term><literal>HASHES</literal></term>
    <listitem>
     <para>
     Indicates this operator can support a hash join. Can only be set
when the operator does not support a hash join.
     </para>
    </listitem>
   </varlistentry>

   <varlistentry>
    <term><literal>MERGES</literal></term>
    <listitem>
     <para>
     Indicates this operator can support a merge join. Can only be set
when the operator does not support a merge join.
     </para>
    </listitem>
   </varlistentry>
------------------------
if the operator cannot support hash/merge join, it can't do ALTER
OPERATOR oprname(left_arg, right_arg) SET (HASHES/MERGES = false);

I think it means:
Can only be set when the operator does support a hash/merge join. Once
set to true, it cannot be reset to false.

  10. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-09-28T20:18:34Z

    Tommy Pavlicek <tommypav122@gmail.com> writes:
> I've attached a new version of the ALTER OPERATOR patch that allows
> no-ops. It should be ready to review now.

I got around to looking through this finally (sorry about the delay).
I'm mostly on board with the functionality, with the exception that
I don't see why we should allow ALTER OPERATOR to cause new shell
operators to be created.  The argument for allowing that in CREATE
OPERATOR was mainly to allow a linked pair of operators to be created
without a lot of complexity (specifically, being careful to specify
the commutator or negator linkage only in the second CREATE, which
is a rule that requires another exception for a self-commutator).
However, if you're using ALTER OPERATOR then you might as well create
both operators first and then link them with an ALTER command.
In fact, I don't really see a use-case where the operators wouldn't
both exist; isn't this feature mainly to allow retrospective
correction of omitted linkages?  So I think allowing ALTER to create a
second operator is more likely to allow mistakes to sneak by than to
do anything useful --- and they will be mistakes you can't correct
except by starting over.  I'd even question whether we want to let
ALTER establish a linkage to an existing shell operator, rather than
insisting you turn it into a valid operator via CREATE first.

If we implement it with that restriction then I don't think the
refactorization done in 0001 is correct, or at least not ideal.

(In any case, it seems like a bad idea that the command reference
pages make no mention of this stuff about shell operators.  It's
explained in 38.15. Operator Optimization Information, but it'd
be worth at least alluding to that section here.  Or maybe we
should move that info to CREATE OPERATOR?)

More generally, you muttered something about 0001 fixing some
existing bugs, but if so I can't see those trees for the forest of
refactorization.  I'd suggest splitting any bug fixes apart from
the pure-refactorization step.

			regards, tom lane

  11. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tommy Pavlicek <tommypav122@gmail.com> — 2023-10-10T20:12:50Z

    On Thu, Sep 28, 2023 at 9:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Tommy Pavlicek <tommypav122@gmail.com> writes:
> > I've attached a new version of the ALTER OPERATOR patch that allows
> > no-ops. It should be ready to review now.
>
> I got around to looking through this finally (sorry about the delay).
> I'm mostly on board with the functionality, with the exception that
> I don't see why we should allow ALTER OPERATOR to cause new shell
> operators to be created.  The argument for allowing that in CREATE
> OPERATOR was mainly to allow a linked pair of operators to be created
> without a lot of complexity (specifically, being careful to specify
> the commutator or negator linkage only in the second CREATE, which
> is a rule that requires another exception for a self-commutator).
> However, if you're using ALTER OPERATOR then you might as well create
> both operators first and then link them with an ALTER command.
> In fact, I don't really see a use-case where the operators wouldn't
> both exist; isn't this feature mainly to allow retrospective
> correction of omitted linkages?  So I think allowing ALTER to create a
> second operator is more likely to allow mistakes to sneak by than to
> do anything useful --- and they will be mistakes you can't correct
> except by starting over.  I'd even question whether we want to let
> ALTER establish a linkage to an existing shell operator, rather than
> insisting you turn it into a valid operator via CREATE first.
>
> If we implement it with that restriction then I don't think the
> refactorization done in 0001 is correct, or at least not ideal.
>
> (In any case, it seems like a bad idea that the command reference
> pages make no mention of this stuff about shell operators.  It's
> explained in 38.15. Operator Optimization Information, but it'd
> be worth at least alluding to that section here.  Or maybe we
> should move that info to CREATE OPERATOR?)
>
> More generally, you muttered something about 0001 fixing some
> existing bugs, but if so I can't see those trees for the forest of
> refactorization.  I'd suggest splitting any bug fixes apart from
> the pure-refactorization step.
>
>                         regards, tom lane

Thanks Tom.

The rationale behind the shell operator and that part of section 38.15
of the documentation had escaped me, but what you're saying makes
complete sense. Based on your comments, I've made some changes:

1. I've isolated the bug fixes (fixing the error message and
disallowing self negation when filling in a shell operator) into
0001-bug-fixes-v3.patch.
2. I've scaled back most of the refactoring as I agree it no longer makes sense.
3. I updated the logic to prevent the creation of or linking to shell operators.
4. I made further updates to the documentation including referencing
38.15 directly in the CREATE and ALTER pages (It's easy to miss if
only 38.14 is referenced) and moved the commentary about creating
commutators and negators into the CREATE section as with the the ALTER
changes it now seems specific to CREATE. I didn't move the rest of
38.15 as I think this applies to both CREATE and ALTER.

I did notice one further potential bug. When creating an operator and
adding a commutator, PostgreSQL only links the commutator back to the
operator if the commutator has no commutator of its own, but the
create operation succeeds regardless of whether this linkage happens.

In other words, if A and B are a pair of commutators and one creates
another operator, C, with A as its commutator, then C will link to A,
but A will still link to B (and B to A). It's not clear to me if this
in itself is a problem, but unless I've misunderstood something
operator C must be the same as B so it implies an error by the user
and there could also be issues if A was deleted since C would continue
to refer to the deleted A.

The same applies for negators and alter operations.

Do you know if this behaviour is intentional or if I've missed
something because it seems undesirable to me. If it is a bug, then I
think I can see how to fix it, but wanted to ask before making any
changes.

On Mon, Sep 25, 2023 at 11:52 AM jian he <jian.universality@gmail.com> wrote:
>
> /*
>  * AlterOperator
>  * routine implementing ALTER OPERATOR <operator> SET (option = ...).
>  *
>  * Currently, only RESTRICT and JOIN estimator functions can be changed.
>  */
> ObjectAddress
> AlterOperator(AlterOperatorStmt *stmt)
>
> The above comment needs to change, other than that, it passed the
> test, works as expected.

Thanks, added a comment.

> Can only be set when the operator does support a hash/merge join. Once
> set to true, it cannot be reset to false.

Yes, I updated the wording. Is it clearer now?

  12. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-10T20:32:07Z

    Tommy Pavlicek <tommypav122@gmail.com> writes:
> I did notice one further potential bug. When creating an operator and
> adding a commutator, PostgreSQL only links the commutator back to the
> operator if the commutator has no commutator of its own, but the
> create operation succeeds regardless of whether this linkage happens.

> In other words, if A and B are a pair of commutators and one creates
> another operator, C, with A as its commutator, then C will link to A,
> but A will still link to B (and B to A). It's not clear to me if this
> in itself is a problem, but unless I've misunderstood something
> operator C must be the same as B so it implies an error by the user
> and there could also be issues if A was deleted since C would continue
> to refer to the deleted A.

Yeah, it'd make sense to tighten that up.  Per the discussion so far,
we should not allow an operator's commutator/negator links to change
once set, so overwriting the existing link with a different value
would be wrong.  But allowing creation of the new operator to proceed
with a different outcome than expected isn't good either.  I think
we should start throwing an error for that.

			regards, tom lane

  13. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tommy Pavlicek <tommypav122@gmail.com> — 2023-10-11T15:11:00Z

    On Tue, Oct 10, 2023 at 9:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Tommy Pavlicek <tommypav122@gmail.com> writes:
> > I did notice one further potential bug. When creating an operator and
> > adding a commutator, PostgreSQL only links the commutator back to the
> > operator if the commutator has no commutator of its own, but the
> > create operation succeeds regardless of whether this linkage happens.
>
> > In other words, if A and B are a pair of commutators and one creates
> > another operator, C, with A as its commutator, then C will link to A,
> > but A will still link to B (and B to A). It's not clear to me if this
> > in itself is a problem, but unless I've misunderstood something
> > operator C must be the same as B so it implies an error by the user
> > and there could also be issues if A was deleted since C would continue
> > to refer to the deleted A.
>
> Yeah, it'd make sense to tighten that up.  Per the discussion so far,
> we should not allow an operator's commutator/negator links to change
> once set, so overwriting the existing link with a different value
> would be wrong.  But allowing creation of the new operator to proceed
> with a different outcome than expected isn't good either.  I think
> we should start throwing an error for that.
>
>                         regards, tom lane

Thanks.

I've added another patch (0002-require_unused_neg_com-v1.patch) that
prevents using a commutator or negator that's already part of a pair.
The only other changes from my email yesterday are that in the ALTER
command I moved the post alter hook to after OperatorUpd and the
addition of tests to verify that we can't use an existing commutator
or negator with the ALTER command.

I believe this can all be looked at again.

Cheers,
Tommy

  14. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    jian he <jian.universality@gmail.com> — 2023-10-12T03:53:53Z

    errmsg("operator attribute \"negator\" cannot be changed if it has
already been set")));
I feel like the above message is not very helpful.

Something like the following may be more helpful for diagnosis.
errmsg("operator %s's attribute \"negator\" cannot be changed if it
has already been set", operatorname)));

when I "git apply", I've noticed some minor whitespace  warning.

Other than that, it looks fine.

  15. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-20T16:33:03Z

    jian he <jian.universality@gmail.com> writes:
> errmsg("operator attribute \"negator\" cannot be changed if it has
> already been set")));
> I feel like the above message is not very helpful.

I think it's okay to be concise about this as long as the operator
we're referring to is the target of the ALTER.  I agree that when
we're complaining about some *other* operator, we'd better spell
out which one we mean, and I made some changes to the patch to
improve that.

Pushed after a round of editorialization -- mostly cosmetic
stuff, except for tweaking some error messages.  I shortened the
test cases a bit too, as I thought they were somewhat excessive
to have as a permanent thing.

			regards, tom lane

  16. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Christoph Berg <myon@debian.org> — 2023-10-24T13:51:16Z

    Re: Tommy Pavlicek
> I've added another patch (0002-require_unused_neg_com-v1.patch) that
> prevents using a commutator or negator that's already part of a pair.

Hmm. I agree with the general idea of adding sanity checks, but this
might be overzealous:

This change is breaking pgsphere which has <@ @> operator pairs, but
for historical reasons also includes alternative spellings of these
operators (both called @ with swapped operand types) which now
explodes because we can't add them with the "proper" commutator and
negators declared (which point to the canonical <@ @> !<@ !@>
operators).

https://github.com/postgrespro/pgsphere/blob/master/pgs_moc_compat.sql.in

We might be able to simply delete the @ operators, but doesn't this
new check break the general possibility to have more than one spelling
for the same operator?

Christoph

  17. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-24T15:16:54Z

    Christoph Berg <myon@debian.org> writes:
> This change is breaking pgsphere which has <@ @> operator pairs, but
> for historical reasons also includes alternative spellings of these
> operators (both called @ with swapped operand types) which now
> explodes because we can't add them with the "proper" commutator and
> negators declared (which point to the canonical <@ @> !<@ !@>
> operators).

Should have guessed that somebody might be depending on the previous
squishy behavior.  Still, I can't see how the above situation is a
good idea.  Commutators/negators should come in pairs, not have
completely random links.  I think it's only accidental that this
setup isn't triggering other strange behavior.

> We might be able to simply delete the @ operators, but doesn't this
> new check break the general possibility to have more than one spelling
> for the same operator?

You can have more than one operator atop the same function.
But why didn't you make the @ operators commutators of each other,
rather than this mess?

			regards, tom lane

  18. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Christoph Berg <myon@debian.org> — 2023-10-24T15:31:43Z

    Re: Tom Lane
> > We might be able to simply delete the @ operators, but doesn't this
> > new check break the general possibility to have more than one spelling
> > for the same operator?
> 
> You can have more than one operator atop the same function.
> But why didn't you make the @ operators commutators of each other,
> rather than this mess?

Historical something.

You are right that the commutators could be fixed that way, but the
negators are a different question. There is no legacy spelling for
these.

Anyway, if this doesn't raise any "oh we didn't think of this"
concerns, we'll just remove the old operators in pgsphere.

Christoph

  19. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-24T15:42:15Z

    Christoph Berg <myon@debian.org> writes:
> Anyway, if this doesn't raise any "oh we didn't think of this"
> concerns, we'll just remove the old operators in pgsphere.

Well, the idea was exactly to forbid that sort of setup.
However, if we get sufficient pushback maybe we should
reconsider --- for example, maybe it'd be sane to enforce
the restriction in ALTER but not CREATE?

I'm inclined to wait and see if there are more complaints.

			regards, tom lane

  20. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tommy Pavlicek <tommypav122@gmail.com> — 2023-10-31T16:10:34Z

    On Fri, Oct 20, 2023 at 5:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Pushed after a round of editorialization -- mostly cosmetic
> stuff, except for tweaking some error messages.  I shortened the
> test cases a bit too, as I thought they were somewhat excessive
> to have as a permanent thing.

Thanks Tom.

On Tue, Oct 24, 2023 at 2:51 PM Christoph Berg <myon@debian.org> wrote:
>
> Re: Tommy Pavlicek
> > I've added another patch (0002-require_unused_neg_com-v1.patch) that
> > prevents using a commutator or negator that's already part of a pair.
>
> Hmm. I agree with the general idea of adding sanity checks, but this
> might be overzealous:

I can't add much beyond what Tom said, but I think this does go a bit
beyond a sanity check. I forgot to mention it in my previous message,
but the main reason I noticed this was because the DELETE operator
code cleans up commutator and negator links to the operator being
deleted and that code expects each to be part of exactly a pair.

  21. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Christoph Berg <myon@debian.org> — 2023-10-31T16:15:52Z

    Re: Tom Lane
> Well, the idea was exactly to forbid that sort of setup.

Fwiw, pgsphere has remove the problematic operators now:

https://github.com/postgrespro/pgsphere/commit/e810f5ddd827881b06a92a303c5c9fbf997b892e

> However, if we get sufficient pushback maybe we should
> reconsider --- for example, maybe it'd be sane to enforce
> the restriction in ALTER but not CREATE?

Hmm, that seems backwards, I'd expect that CREATE might have some
checks that could circumvent using ALTER if I really insisted. If
CREATE can create things that I can't reach by ALTERing existing other
things, that's weird.

Let's keep it like it is now in PG17.

Christoph

  22. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Christoph Berg <myon@debian.org> — 2024-09-12T15:46:55Z

    Re: To Tom Lane
> Let's keep it like it is now in PG17.

Late followup news: This feature has actually found a bug in
postgresql-debversion:

 CREATE OPERATOR > (
   LEFTARG = debversion,
   RIGHTARG = debversion,
   COMMUTATOR = <,
-  NEGATOR = >=,
+  NEGATOR = <=,
   RESTRICT = scalargtsel,
   JOIN = scalargtjoinsel
 );

https://salsa.debian.org/postgresql/postgresql-debversion/-/commit/8ef08ccbea1438468249b0e94048b1a8a25fc625#000e84a71f8a28b762658375c194b25d529336f3

So, thanks!

Christoph

  23. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Michael Banck <mbanck@gmx.net> — 2024-09-24T22:07:59Z

    Hi,

On Tue, Oct 24, 2023 at 11:42:15AM -0400, Tom Lane wrote:
> Christoph Berg <myon@debian.org> writes:
> > Anyway, if this doesn't raise any "oh we didn't think of this"
> > concerns, we'll just remove the old operators in pgsphere.
> 
> Well, the idea was exactly to forbid that sort of setup.
> However, if we get sufficient pushback maybe we should
> reconsider --- for example, maybe it'd be sane to enforce
> the restriction in ALTER but not CREATE?
> 
> I'm inclined to wait and see if there are more complaints.

FWIW, rdkit also fails, but that seems to be an ancient thing as well:

https://github.com/rdkit/rdkit/issues/7843

I guess there's no way to make that error a bit more helpful, like
printing out the offenbding SQL command, presumably because we are
loding an extension?


Michael

  24. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Christoph Berg <myon@debian.org> — 2024-09-26T14:51:54Z

    Re: Michael Banck
> I guess there's no way to make that error a bit more helpful, like
> printing out the offenbding SQL command, presumably because we are
> loding an extension?

I wish there was. The error reporting from failing extension scripts
is really bad with no context at all, it has hit me a few times in the
past already.

Christoph

  25. Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-26T15:51:47Z

    Christoph Berg <myon@debian.org> writes:
> I wish there was. The error reporting from failing extension scripts
> is really bad with no context at all, it has hit me a few times in the
> past already.

Nobody's spent any work on that :-(.  A really basic reporting
facility is not hard to add, as in the attached finger exercise.
The trouble with it can be explained by showing what I get after
intentionally breaking a script file command:

regression=# create extension cube;
ERROR:  syntax error at or near "CREAT"
LINE 16: CREAT FUNCTION cube_send(cube)
         ^
QUERY:  /* contrib/cube/cube--1.4--1.5.sql */

-- complain if script is sourced in psql, rather than via ALTER EXTENSION


-- Remove @ and ~
DROP OPERATOR @ (cube, cube);
DROP OPERATOR ~ (cube, cube);

-- Add binary input/output handlers
CREATE FUNCTION cube_recv(internal)
RETURNS cube
AS '$libdir/cube'
LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;

CREAT FUNCTION cube_send(cube)
RETURNS bytea
AS '$libdir/cube'
LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;

ALTER TYPE cube SET ( RECEIVE = cube_recv, SEND = cube_send );

CONTEXT:  extension script file "/home/postgres/install/share/extension/cube--1.4--1.5.sql"

So the first part of that is great, but if your script file is
large you probably won't be happy about having the whole thing
repeated in the "QUERY" field.  So this needs some work on
user-friendliness.

I'm inclined to think that maybe we'd be best off keeping the server
end of it straightforward, and trying to teach psql to abbreviate the
QUERY field in a useful way.  IIRC you get this same type of problem
with very large SQL-language functions and suchlike.

Also, I believe this doesn't help much for non-syntax errors
(those that aren't reported with an error location).  It might
be interesting to use the RawStmt.stmt_location/stmt_len fields
for the current parsetree to identify what to report, but
I've not dug further than this.

			regards, tom lane

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

    Christoph Berg <myon@debian.org> — 2024-09-27T12:17:15Z

    Re: Tom Lane
> So the first part of that is great, but if your script file is
> large you probably won't be happy about having the whole thing
> repeated in the "QUERY" field.  So this needs some work on
> user-friendliness.

Does this really have to be addressed? It would be way better than it
is now, and errors during extension creation are rare and mostly for
developers only, so it doesn't have to be pretty.

> I'm inclined to think that maybe we'd be best off keeping the server
> end of it straightforward, and trying to teach psql to abbreviate the
> QUERY field in a useful way.  IIRC you get this same type of problem
> with very large SQL-language functions and suchlike.

I'd treat this as a separate patch, if it's considered to be a good
idea.

Christoph

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-27T16:31:08Z

    Christoph Berg <myon@debian.org> writes:
> Re: Tom Lane
>> So the first part of that is great, but if your script file is
>> large you probably won't be happy about having the whole thing
>> repeated in the "QUERY" field.  So this needs some work on
>> user-friendliness.

> Does this really have to be addressed? It would be way better than it
> is now, and errors during extension creation are rare and mostly for
> developers only, so it doesn't have to be pretty.

Perhaps.  I spent a little more effort on this and added code to
report errors that don't come with an error location.  On those,
we don't have any constraints about what to report in the QUERY
field, so I made it trim the string to just the current query
within the script, which makes things quite a bit better.  You
can see the results in the test_extensions regression test changes.

(It might be worth some effort to trim away comments appearing
just before a command, but I didn't tackle that here.)

			regards, tom lane

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

    Christoph Berg <myon@debian.org> — 2024-09-27T17:31:19Z

    Re: Tom Lane
> Perhaps.  I spent a little more effort on this and added code to
> report errors that don't come with an error location.  On those,
> we don't have any constraints about what to report in the QUERY
> field, so I made it trim the string to just the current query
> within the script, which makes things quite a bit better.  You
> can see the results in the test_extensions regression test changes.

That looks very good me, thanks!

> (It might be worth some effort to trim away comments appearing
> just before a command, but I didn't tackle that here.)

The "error when psql" comments do look confusing, but I guess in other
places the comment just before the query adds valuable context, so I'd
say leaving the comments in is ok.

Christoph

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-27T17:54:44Z

    Christoph Berg <myon@debian.org> writes:
> Re: Tom Lane
>> (It might be worth some effort to trim away comments appearing
>> just before a command, but I didn't tackle that here.)

> The "error when psql" comments do look confusing, but I guess in other
> places the comment just before the query adds valuable context, so I'd
> say leaving the comments in is ok.

It looks like if we did want to suppress that, the right fix is to
make gram.y track statement start locations more honestly, as in
0002 attached (0001 is the same as before).  This'd add a few cycles
per grammar nonterminal reduction, which is kind of annoying but
probably is negligible in the grand scheme of things.  Still, I'd
not propose it just for this.  But if memory serves, we've had
previous complaints about pg_stat_statements failing to strip
leading comments from queries, and this'd fix that.  I think it
likely also improves error cursor positioning for cases involving
empty productions --- I'm a tad surprised that no other regression
cases changed.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-28T19:45:36Z

    I wrote:
> It looks like if we did want to suppress that, the right fix is to
> make gram.y track statement start locations more honestly, as in
> 0002 attached (0001 is the same as before).

I did a little bit of further work on this:

* I ran some performance checks and convinced myself that indeed
the more complex definition of YYLLOC_DEFAULT has no measurable
cost compared to the overall runtime of raw_parser(), never mind
later processing.  So I see no reason not to go ahead with that
change.  I swapped the two patches to make that 0001, and added
a regression test illustrating its effect on pg_stat_statements.
(Without the gram.y change, the added slash-star comment shows
up in the pg_stat_statements output, which is surely odd.)

* I concluded that the best way to report the individual statement
when we're able to do that is to include it in an errcontext()
message, similar to what spi.c's _SPI_error_callback does.
Otherwise it interacts badly if some more-tightly-nested error
context function has already set up an "internal error query",
as for example SQL function errors will do if you enable
check_function_bodies = on.

So here's v3, now with commit messages and regression tests.
I feel like this is out of the proof-of-concept stage and
might now actually be committable.  There's still a question
of whether reporting the whole script as the query is OK when
we have a syntax error, but I have no good ideas as to how to
make that terser.  And I think you're right that we shouldn't let
perfection stand in the way of making things noticeably better.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-08T20:17:58Z

    I wrote:
> ... There's still a question
> of whether reporting the whole script as the query is OK when
> we have a syntax error, but I have no good ideas as to how to
> make that terser.

I had an idea about this: we can use a pretty simple heuristic
such as "break at semicolon-newline sequences".  That could fail
and show you just a fragment of a statement, but that still seems
better than showing a whole extension script.  We can ameliorate
the problem that we might not show enough to clearly identify
what failed by including a separate line number counter.
In the attached v4 I included that in the context line that
reports the script file, eg

+CONTEXT:  SQL statement "CREATE OR REPLACE FUNCTION ext_cor_func() RETURNS text
+  AS $$ SELECT 'ext_cor_func: from extension'::text $$ LANGUAGE sql"
+extension script file "test_ext_cor--1.0.sql", near line 8

This way seems a whole lot more usable when dealing with a
large extension script.

			regards, tom lane

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2024-10-11T13:37:24Z

    Hi

út 8. 10. 2024 v 22:18 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> I wrote:
> > ... There's still a question
> > of whether reporting the whole script as the query is OK when
> > we have a syntax error, but I have no good ideas as to how to
> > make that terser.
>
> I had an idea about this: we can use a pretty simple heuristic
> such as "break at semicolon-newline sequences".  That could fail
> and show you just a fragment of a statement, but that still seems
> better than showing a whole extension script.  We can ameliorate
> the problem that we might not show enough to clearly identify
> what failed by including a separate line number counter.
> In the attached v4 I included that in the context line that
> reports the script file, eg
>
> +CONTEXT:  SQL statement "CREATE OR REPLACE FUNCTION ext_cor_func()
> RETURNS text
> +  AS $$ SELECT 'ext_cor_func: from extension'::text $$ LANGUAGE sql"
> +extension script file "test_ext_cor--1.0.sql", near line 8
>
> This way seems a whole lot more usable when dealing with a
> large extension script.
>

I tested it and it is working nicely.  I tested it against Orafce and I
found an interesting point. The body of plpgsql functions is not checked.

Do you know the reason?

Regards

Pavel



>                         regards, tom lane
>
>

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-11T16:08:24Z

    Pavel Stehule <pavel.stehule@gmail.com> writes:
> I tested it and it is working nicely.  I tested it against Orafce and I
> found an interesting point. The body of plpgsql functions is not checked.
> Do you know the reason?

In execute_extension_script():

    /*
     * Similarly disable check_function_bodies, to ensure that SQL functions
     * won't be parsed during creation.
     */
    if (check_function_bodies)
        (void) set_config_option("check_function_bodies", "off",
                                 PGC_USERSET, PGC_S_SESSION,
                                 GUC_ACTION_SAVE, true, 0, false);

I wondered if we should reconsider that, but I'm afraid we'd be more
likely to break working extensions than to do anything helpful.
An extension author who wants that can do what I did in the patch's
test cases: manually turn check_function_bodies on in the extension
script.

			regards, tom lane

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2024-10-11T17:39:37Z

    pá 11. 10. 2024 v 18:08 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > I tested it and it is working nicely.  I tested it against Orafce and I
> > found an interesting point. The body of plpgsql functions is not checked.
> > Do you know the reason?
>
> In execute_extension_script():
>
>     /*
>      * Similarly disable check_function_bodies, to ensure that SQL
> functions
>      * won't be parsed during creation.
>      */
>     if (check_function_bodies)
>         (void) set_config_option("check_function_bodies", "off",
>                                  PGC_USERSET, PGC_S_SESSION,
>                                  GUC_ACTION_SAVE, true, 0, false);
>
> I wondered if we should reconsider that, but I'm afraid we'd be more
> likely to break working extensions than to do anything helpful.
> An extension author who wants that can do what I did in the patch's
> test cases: manually turn check_function_bodies on in the extension
> script.
>

ok,

Pavel

>
>                         regards, tom lane
>

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

    jian he <jian.universality@gmail.com> — 2024-10-12T07:32:40Z

    On Wed, Oct 9, 2024 at 4:18 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>

> In the attached v4


in the upper code two branch, both will call CleanQuerytext
so in script_error_callback

+ /*
+ * 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)
+ {
+ int linenumber = 1;
+
+ for (query = callback_arg->sql; *query; query++)
+ {
+ if (--location < 0)
+ break;
+ if (*query == '\n')
+ linenumber++;
+ }
+ errcontext("extension script file \"%s\", near line %d",
+   lastslash, linenumber);
+ }
+ else
+ errcontext("extension script file \"%s\"", lastslash);


+ /*
+ * 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?

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2024-10-13T16:48:03Z

    so 12. 10. 2024 v 9:33 odesílatel jian he <jian.universality@gmail.com>
napsal:

> On Wed, Oct 9, 2024 at 4:18 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
>
> > In the attached v4
>
>
> in the upper code two branch, both will call CleanQuerytext
> so in script_error_callback
>
> + /*
> + * 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)
> + {
> + int linenumber = 1;
> +
> + for (query = callback_arg->sql; *query; query++)
> + {
> + if (--location < 0)
> + break;
> + if (*query == '\n')
> + linenumber++;
> + }
> + errcontext("extension script file \"%s\", near line %d",
> +   lastslash, linenumber);
> + }
> + else
> + errcontext("extension script file \"%s\"", lastslash);
>
>
> + /*
> + * 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

Regards

Pavel

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-13T17:13:51Z

    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.  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.

			regards, tom lane

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

    jian he <jian.universality@gmail.com> — 2024-10-14T03:38:47Z

    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.

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-14T03:50:58Z

    jian he <jian.universality@gmail.com> writes:
> So if we are in script_error_callback
> `int            location = callback_arg->stmt_location;`
> location >= 0 will be always true?

No, not if the grammar threw an error.

			regards, tom lane

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2024-10-14T14:30:49Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-14T16:45:27Z

    jian he <jian.universality@gmail.com> writes:
> On Mon, Oct 14, 2024 at 1:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.

Yes, I would expect that any RawStmt we see here will have valid
stmt_location.  What you seem to be missing is that an error could
be thrown from

>     raw_parsetree_list = pg_parse_query(sql);

before execute_sql_string reaches its loop over RawStmts.  In that
case we'll reach script_error_callback with callback_arg.stmt_location
still being -1.

> pg_parse_query(sql) doesn't use script_error_callback.

Eh?  We've put that on the error context callback stack.
It is not pg_parse_query's decision whether it will be called.

			regards, tom lane

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

    jian he <jian.universality@gmail.com> — 2024-10-15T07:10:32Z

    On Tue, Oct 15, 2024 at 12:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> jian he <jian.universality@gmail.com> writes:
> > On Mon, Oct 14, 2024 at 1:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> 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.
>
> Yes, I would expect that any RawStmt we see here will have valid
> stmt_location.  What you seem to be missing is that an error could
> be thrown from
>
> >     raw_parsetree_list = pg_parse_query(sql);
>
> before execute_sql_string reaches its loop over RawStmts.  In that
> case we'll reach script_error_callback with callback_arg.stmt_location
> still being -1.
>
Thanks for your reply.
I think I got it.



        if (location < 0 || syntaxerrposition < location ||
            (len > 0 && syntaxerrposition > location + len))
        {
            int            slen = strlen(query);
            location = len = 0;
            .....
        }
        else
            elog(INFO, "should not reached here");

just found out the"elog(INFO, "should not reached here");" part never reached.
in script_error_callback, for syntax error (error while parsing the raw string.)
we can bookkeeping the line number.

so we don't need to loop it again for syntax error in:
        for (query = callback_arg->sql; *query; query++)
        {
            if (--location < 0)
                break;
            if (*query == '\n')
                linenumber++;
        }
since we already did it in loop
"for (int loc = 0; loc < slen; loc++)"


i guess, we don't need performance in script_error_callback,
but in script_error_callback arrange code seperate  syntax error(raw
parser) and other error seems good.
please check the attached minor refactor.

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-16T18:17:58Z

    jian he <jian.universality@gmail.com> writes:
> just found out the"elog(INFO, "should not reached here");" part never reached.

You didn't check any of the cases we were discussing I guess?
(That is, places in gram.y that throw an error without a
parser_errposition call.)

Note that even if we fix all of those and keep them fixed, we still
couldn't assume the case is unreachable, because gram.y isn't
self-contained.  For instance, if we hit out-of-memory during raw
parsing, the OOM error out of mcxt.c isn't going to provide a syntax
error position.  I'm not too concerned about doing better than what
the patch does now (i.e. nothing) in such edge cases, but we can't
do worse.

> i guess, we don't need performance in script_error_callback,
> but in script_error_callback arrange code seperate  syntax error(raw
> parser) and other error seems good.
> please check the attached minor refactor.

I do not think that's an improvement.  It's more complicated and
less readable, and I don't see why we need to squeeze more
performance out of this error-reporting path that should never
be taken in production.

			regards, tom lane

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2024-10-22T02:54:39Z

    Hi

pá 11. 10. 2024 v 19:39 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

>
>
> pá 11. 10. 2024 v 18:08 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>
>> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> > I tested it and it is working nicely.  I tested it against Orafce and I
>> > found an interesting point. The body of plpgsql functions is not
>> checked.
>> > Do you know the reason?
>>
>> In execute_extension_script():
>>
>>     /*
>>      * Similarly disable check_function_bodies, to ensure that SQL
>> functions
>>      * won't be parsed during creation.
>>      */
>>     if (check_function_bodies)
>>         (void) set_config_option("check_function_bodies", "off",
>>                                  PGC_USERSET, PGC_S_SESSION,
>>                                  GUC_ACTION_SAVE, true, 0, false);
>>
>> I wondered if we should reconsider that, but I'm afraid we'd be more
>> likely to break working extensions than to do anything helpful.
>> An extension author who wants that can do what I did in the patch's
>> test cases: manually turn check_function_bodies on in the extension
>> script.
>>
>
> ok,
>
>
I tested this patch and I didn't find any issue. The possibility to show
errors inside extensions more precisely is very useful.

compilation without problems, all tests passed

I'll mark this patch as ready for committer.

Regards

Pavel




> Pavel
>
>>
>>                         regards, tom lane
>>
>

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-22T15:37:03Z

    Pavel Stehule <pavel.stehule@gmail.com> writes:
> I'll mark this patch as ready for committer.

Pushed then.  Thanks for reviewing!

			regards, tom lane

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2024-10-22T18:35:19Z

    út 22. 10. 2024 v 17:37 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > I'll mark this patch as ready for committer.
>
> Pushed then.  Thanks for reviewing!
>

Thank you for this patch. It is really practical

Regards

Pavel


>
>                         regards, tom lane
>

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-24T15:47:52Z

    In the no-good-deed-goes-unpunished department: buildfarm member
hamerkop doesn't like this patch [1].  The diffs look like

@@ -77,7 +77,7 @@
 ERROR:  syntax error at or near "FUNCTIN"
 LINE 1: CREATE FUNCTIN my_erroneous_func(int) RETURNS int LANGUAGE S...
                ^
-QUERY:  CREATE FUNCTIN my_erroneous_func(int) RETURNS int LANGUAGE SQL
+QUERY:  CREATE FUNCTIN my_erroneous_func(int) RETURNS int LANGUAGE SQL
 AS $$ SELECT $1 + 1 $$;
 CONTEXT:  extension script file "test_ext7--2.0--2.1bad.sql", near line 10
 alter extension test_ext7 update to '2.2bad';

It's hard to be totally sure from the web page, but I suppose what
is happening is that the newline within the quoted query fragment
is represented as "\r\n" not just "\n".  (I wonder why the cfbot
failed to detect this; there must be more moving parts involved
than just "it's Windows".)

The reason why this is happening seems fairly clear: extension.c's
read_whole_file() opens the script file with PG_BINARY_R, preventing
Windows' libc from hiding DOS-style newlines from us, even though the
git checkout on that machine is evidently using DOS newlines.

That seems like a rather odd choice.  Elsewhere in the same module,
parse_extension_control_file() opens control files with plain "r",
so that those act differently.  I checked the git history and did
not learn much.  The original extensions commit d9572c4e3 implemented
reading with a call to read_binary_file(), and we seem to have just
carried that behavioral decision forward through various refactorings.
I don't recall if there was an intentional choice to use binary read
or that was just a random decision to use an available function.

So what I'd like to do to fix this is to change

-	if ((file = AllocateFile(filename, PG_BINARY_R)) == NULL)
+	if ((file = AllocateFile(filename, "r")) == NULL)

The argument against that is it creates a nonzero chance of breaking
somebody's extension script file on Windows.  But there's a
counter-argument that it might *prevent* bugs on Windows, by making
script behavior more nearly identical to what it is on not-Windows.
So I think that's kind of a wash.

Other approaches we could take with perhaps-smaller blast radii
include making script_error_callback() trim \r's out of the quoted
text (ugly) or creating a variant expected-file (hard to maintain,
and I wonder if it'd survive git-related newline munging).

Thoughts?

			regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hamerkop&dt=2024-10-23%2011%3A00%3A37

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-27T17:41:54Z

    I wrote:
> In the no-good-deed-goes-unpunished department: buildfarm member
> hamerkop doesn't like this patch [1].  The diffs look like
> ...
> So what I'd like to do to fix this is to change
> -	if ((file = AllocateFile(filename, PG_BINARY_R)) == NULL)
> +	if ((file = AllocateFile(filename, "r")) == NULL)

Well, that didn't fix it :-(.  I went so far as to extract the raw log
files from the buildfarm database, and what they show is that there is
absolutely no difference between the lines diff is claiming are
different:

-QUERY:  CREATE FUNCTIN my_erroneous_func(int) RETURNS int LANGUAGE SQL\r\n
+QUERY:  CREATE FUNCTIN my_erroneous_func(int) RETURNS int LANGUAGE SQL\r\n

It's the same both before and after 924e03917, which made the code
change depicted above, so that didn't help.

So I'm pretty baffled.  I suppose the expected and result files must
actually be different, and something in subsequent processing is
losing the difference before it gets to the buildfarm database.
But I don't have the ability to debug that from here.  Does anyone
with access to hamerkop want to poke into this?

Without additional information, the only thing I can think of that
I have any confidence will eliminate these failures is to reformat
the affected test cases so that they produce just a single line of
output.  That's kind of annoying from a functionality-coverage point
of view, but I'm not sure avoiding it is worth moving mountains for.

In any case, I'm disinclined to revert 924e03917.  It seems like a
good change on balance, even if it failed to fix whatever is
happening on hamerkop.

			regards, tom lane

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2024-10-27T17:56:59Z

    ne 27. 10. 2024 v 18:42 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> I wrote:
> > In the no-good-deed-goes-unpunished department: buildfarm member
> > hamerkop doesn't like this patch [1].  The diffs look like
> > ...
> > So what I'd like to do to fix this is to change
> > -     if ((file = AllocateFile(filename, PG_BINARY_R)) == NULL)
> > +     if ((file = AllocateFile(filename, "r")) == NULL)
>
> Well, that didn't fix it :-(.  I went so far as to extract the raw log
> files from the buildfarm database, and what they show is that there is
> absolutely no difference between the lines diff is claiming are
> different:
>
> -QUERY:  CREATE FUNCTIN my_erroneous_func(int) RETURNS int LANGUAGE SQL\r\n
> +QUERY:  CREATE FUNCTIN my_erroneous_func(int) RETURNS int LANGUAGE SQL\r\n
>
> It's the same both before and after 924e03917, which made the code
> change depicted above, so that didn't help.
>
> So I'm pretty baffled.  I suppose the expected and result files must
> actually be different, and something in subsequent processing is
> losing the difference before it gets to the buildfarm database.
> But I don't have the ability to debug that from here.  Does anyone
> with access to hamerkop want to poke into this?
>
> Without additional information, the only thing I can think of that
> I have any confidence will eliminate these failures is to reformat
> the affected test cases so that they produce just a single line of
> output.  That's kind of annoying from a functionality-coverage point
> of view, but I'm not sure avoiding it is worth moving mountains for.
>
> In any case, I'm disinclined to revert 924e03917.  It seems like a
> good change on balance, even if it failed to fix whatever is
> happening on hamerkop.
>

+1

This is very useful feature

Pavel



>                         regards, tom lane
>

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

    Alexander Law <exclusion@gmail.com> — 2024-10-28T09:00:00Z

    Hello Tom,

27.10.2024 20:41, Tom Lane wrote:
> I wrote:
>> In the no-good-deed-goes-unpunished department: buildfarm member
>> hamerkop doesn't like this patch [1].  The diffs look like
>> ...
>> So what I'd like to do to fix this is to change
>> -	if ((file = AllocateFile(filename, PG_BINARY_R)) == NULL)
>> +	if ((file = AllocateFile(filename, "r")) == NULL)
> Well, that didn't fix it :-(.  I went so far as to extract the raw log
> files from the buildfarm database, and what they show is that there is
> absolutely no difference between the lines diff is claiming are
> different:
>
> -QUERY:  CREATE FUNCTIN my_erroneous_func(int) RETURNS int LANGUAGE SQL\r\n
> +QUERY:  CREATE FUNCTIN my_erroneous_func(int) RETURNS int LANGUAGE SQL\r\n
>
> It's the same both before and after 924e03917, which made the code
> change depicted above, so that didn't help.
>
> So I'm pretty baffled.  I suppose the expected and result files must
> actually be different, and something in subsequent processing is
> losing the difference before it gets to the buildfarm database.
> But I don't have the ability to debug that from here.  Does anyone
> with access to hamerkop want to poke into this?
>
> Without additional information, the only thing I can think of that
> I have any confidence will eliminate these failures is to reformat
> the affected test cases so that they produce just a single line of
> output.  That's kind of annoying from a functionality-coverage point
> of view, but I'm not sure avoiding it is worth moving mountains for.
>

I've managed to reproduce the issue with the core.autocrlf=true git setting
(which sets CR+LF line ending in test_ext7--2.0--2.1bad.sql) and with diff
from msys:
C:\msys64\usr\bin\diff.exe --version
diff (GNU diffutils) 3.8

(Gnu/Win32 Diff [1] doesn't detect those EOL differences and thus the test
doesn't fail.)

I can really see different line endings with hexdump:
hexdump -C ...testrun\test_extensions\regress\regression.diffs
00000230  20 20 20 20 20 20 20 20  5e 0a 2d 51 55 45 52 59  | ^.-QUERY|
00000240  3a 20 20 43 52 45 41 54  45 20 46 55 4e 43 54 49  |: CREATE FUNCTI|
00000250  4e 20 6d 79 5f 65 72 72  6f 6e 65 6f 75 73 5f 66  |N my_erroneous_f|
00000260  75 6e 63 28 69 6e 74 29  20 52 45 54 55 52 4e 53 |unc(int) RETURNS|
00000270  20 69 6e 74 20 4c 41 4e  47 55 41 47 45 20 53 51  | int LANGUAGE SQ|
00000280  4c 0a 2b 51 55 45 52 59  3a 20 20 43 52 45 41 54 |L.+QUERY:  CREAT|
00000290  45 20 46 55 4e 43 54 49  4e 20 6d 79 5f 65 72 72  |E FUNCTIN my_err|
000002a0  6f 6e 65 6f 75 73 5f 66  75 6e 63 28 69 6e 74 29 |oneous_func(int)|
000002b0  20 52 45 54 55 52 4e 53  20 69 6e 74 20 4c 41 4e  | RETURNS int LAN|
000002c0  47 55 41 47 45 20 53 51  4c 0d 0a 20 41 53 20 24  |GUAGE SQL.. AS $|

hexdump -C .../testrun/test_extensions/regress/results/test_extensions.out | grep -C5 FUNCTIN
00000b80  20 5e 0d 0a 51 55 45 52  59 3a 20 20 43 52 45 41  | ^..QUERY:  CREA|
00000b90  54 45 20 46 55 4e 43 54  49 4e 20 6d 79 5f 65 72  |TE FUNCTIN my_er|
00000ba0  72 6f 6e 65 6f 75 73 5f  66 75 6e 63 28 69 6e 74 |roneous_func(int|
00000bb0  29 20 52 45 54 55 52 4e  53 20 69 6e 74 20 4c 41  |) RETURNS int LA|
00000bc0  4e 47 55 41 47 45 20 53  51 4c 0d 0d 0a 41 53 20  |NGUAGE SQL...AS |

whilst
hexdump -C .../src/test/modules/test_extensions/expected/test_extensions.out | grep -C5 FUNCTIN
00000b80  20 5e 0d 0a 51 55 45 52  59 3a 20 20 43 52 45 41  | ^..QUERY:  CREA|
00000b90  54 45 20 46 55 4e 43 54  49 4e 20 6d 79 5f 65 72  |TE FUNCTIN my_er|
00000ba0  72 6f 6e 65 6f 75 73 5f  66 75 6e 63 28 69 6e 74 |roneous_func(int|
00000bb0  29 20 52 45 54 55 52 4e  53 20 69 6e 74 20 4c 41  |) RETURNS int LA|
00000bc0  4e 47 55 41 47 45 20 53  51 4c 0d 0a 41 53 20 24  |NGUAGE SQL..AS $|

It looks like --strip-trailing-cr doesn't work as desired for this diff version.

I've also dumped buf in read_whole_file() and found that in both
PG_BINARY_R and "r" modes the 0d 0a ending is preserved. But it changed
to 0a with the "rt" mode (see [1]), and it makes the test (and the whole
`meson test`) pass for me.

[1] https://gnuwin32.sourceforge.net/packages/diffutils.htm
[2] https://learn.microsoft.com/en-us/cpp/c-runtime-library/file-translation-constants?view=msvc-170

Best regards,
Alexander

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-28T16:06:46Z

    Alexander Lakhin <exclusion@gmail.com> writes:
> 27.10.2024 20:41, Tom Lane wrote:
>>> In the no-good-deed-goes-unpunished department: buildfarm member
>>> hamerkop doesn't like this patch [1].  The diffs look like

> I've managed to reproduce the issue with the core.autocrlf=true git setting
> (which sets CR+LF line ending in test_ext7--2.0--2.1bad.sql) and with diff
> from msys:
> C:\msys64\usr\bin\diff.exe --version
> diff (GNU diffutils) 3.8
> (Gnu/Win32 Diff [1] doesn't detect those EOL differences and thus the test
> doesn't fail.)

Thank you for looking at this!

> I can really see different line endings with hexdump:
> hexdump -C .../testrun/test_extensions/regress/results/test_extensions.out | grep -C5 FUNCTIN
> 00000b80  20 5e 0d 0a 51 55 45 52  59 3a 20 20 43 52 45 41  | ^..QUERY:  CREA|
> 00000b90  54 45 20 46 55 4e 43 54  49 4e 20 6d 79 5f 65 72  |TE FUNCTIN my_er|
> 00000ba0  72 6f 6e 65 6f 75 73 5f  66 75 6e 63 28 69 6e 74 |roneous_func(int|
> 00000bb0  29 20 52 45 54 55 52 4e  53 20 69 6e 74 20 4c 41  |) RETURNS int LA|
> 00000bc0  4e 47 55 41 47 45 20 53  51 4c 0d 0d 0a 41 53 20  |NGUAGE SQL...AS |

Wow.  How are we producing \r\r\n?  I'm not hugely surprised that some
versions of diff might see that as different from a single newline
even with --strip-trailing-cr --- formally, I think that's supposed
to make \r\n match \n, but the extra \r doesn't fit that pattern.

> I've also dumped buf in read_whole_file() and found that in both
> PG_BINARY_R and "r" modes the 0d 0a ending is preserved. But it changed
> to 0a with the "rt" mode (see [1]), and it makes the test (and the whole
> `meson test`) pass for me.

Interesting.  I believe we decided years ago that we didn't need to
use "rt" mode because that was the default on Windows, but was that
a misreading of the documentation?  The link you provided doesn't
give any hint that there are more than two behaviors.

However ... the link you provided also mentions that text mode
includes treating control-Z as EOF; which I'd forgotten, but it
does make it less safe than I thought to use text mode for
reading script files.

What I'm now thinking is that we should revert 924e03917 after
all (that is, go back to using PG_BINARY_R) and instead make
read_whole_file manually squash \r\n to \n if we're on Windows.
Ugly, but I have yet to find anything about that platform that
isn't.

			regards, tom lane

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

    Alexander Law <exclusion@gmail.com> — 2024-10-28T19:00:00Z

    28.10.2024 19:06, Tom Lane wrote:
>> I've also dumped buf in read_whole_file() and found that in both
>> PG_BINARY_R and "r" modes the 0d 0a ending is preserved. But it changed
>> to 0a with the "rt" mode (see [1]), and it makes the test (and the whole
>> `meson test`) pass for me.
> Interesting.  I believe we decided years ago that we didn't need to
> use "rt" mode because that was the default on Windows, but was that
> a misreading of the documentation?  The link you provided doesn't
> give any hint that there are more than two behaviors.
>
> However ... the link you provided also mentions that text mode
> includes treating control-Z as EOF; which I'd forgotten, but it
> does make it less safe than I thought to use text mode for
> reading script files.

I think that this other behavior can be explained by pgwin32_fopen()/
pgwin32_open() coding (O_TEXT assumed implicitly only #ifdef FRONTEND).

Anyway, as you noticed, \x1A injected into test_ext....sql really leads to
the file contents truncation on read (with "rt"), so I agree that using the
text/translation mode here is not an improvement.

> What I'm now thinking is that we should revert 924e03917 after
> all (that is, go back to using PG_BINARY_R) and instead make
> read_whole_file manually squash \r\n to \n if we're on Windows.
> Ugly, but I have yet to find anything about that platform that
> isn't.

Yes, I think this should help.

Best regards,
Alexander