Re: Extension security improvement: Add support for extensions with an owned schema

2024-10-04 Thread Jelte Fennema-Nio
On Fri, 27 Sept 2024 at 14:00, Tomas Vondra  wrote:
> One thing that's not quite clear to me is what's the correct way for
> existing extensions to switch to an "owned schema". Let's say you have
> an extension. How do you transition to this? Can you just add it to the
> control file and then some magic happens?

Sadly no. As currently implemented this feature can only really be
used by new extensions. There is no upgrade path to it for existing
extensions. This is fairly common btw, the only field in the control
file that ever gets used after the taken from the control file for
ALTER EXTENSION is the version field. e.g. if you change the schema
field in the control file of an already installed extension, the
extension will not be moved to the new schema unless you DROP + CREATE
EXTENSION.

After this message I tried messing around a bit with changing an
existing extension to become an owned schema (either with a new
command or as part of ALTER EXTENSION UPDATE). But it's not as trivial
as I hoped for a few reasons:
1. There are multiple states that extensions are currently in all of
which need somewhat different conversion strategies. Specifically:
relocatable/non-relocatable &
schema=pg_catalog/public/actual_extension_schema.
2. There are two important assumptions on the schema for an
owned_schema, which we would need to check on an existing schema
converting it:
   a. The owner of the extension should be the owner of the schema
   b. There are no other objects in the schema appart from the extension.

I'll probably continue some efforts to allow for migration, because I
agree it's useful. But I don't think it's critical for this feature to
be committable. Even if this can only be used by new extensions (that
target PG18+ only), I think that would already be very useful. i.e. if
in 5 years time I don't have to worry about these security pitfalls
for any new extensions that I write then I'll be very happy. Also if
an extension really wants to upgrade to an owned schema, then that
should be possible by doing some manual catalog hackery in its
extension update script, because that's effectively what any automatic
conversion would also do.

> A couple minor comments:
> Doesn't "extension is owned_schema" sound a bit weird?

Updated the docs to address all of your feedback I think.

> Also, perhaps "dedicated_schema" would be better than "owned_schema"? I
> mean, the point is not that it's "owned" by the extension, but that
> there's nothing else in it. But that's nitpicking.

I agree that's probably a better name. Given the amount of effort
necessary to update everything I haven't done that yet. I'll think a
bit if there's a name I like even better in the meantime, and I'm
definitely open to other suggestions.

> 3) src/backend/commands/extension.c
>
> I'm not sure why AlterExtensionNamespace moves the privilege check. Why
> should it not check the privilege for owned schema too?

Because for an owned schema we're not creating objects in an existing
schema. We're only renaming the schema that the extension is already
in using RenameSchema, so there's no point in checking if the user can
create objects in that schema, since the objects are already there.
(RenameSchema also checks internally if the current user is the owner
of the schema)


> Shouldn't binary_upgrade_extension_member still set ext=NULL in the for
> loop, the way the original code does that?

I don't think that's necessary. It was necessary before to set extobj
to NULL, because that variable was set every loop. But now extobj is
scoped to the loop, and ext is only set right before the break. So
either we set ext in the loop and break out of the loop right away, or
ext is still set to the NULL value that's was assigned at the top of
the function.

> The long if conditions might need some indentation, I guess. pgindent
> leaves them like this, but 100 columns seems a bit too much. I'd do a
> line break after each condition, I guess.

Done


v4-0001-Add-support-for-extensions-with-an-owned-schema.patch
Description: Binary data


Re: Converting README documentation to Markdown

2024-10-01 Thread Jelte Fennema-Nio
On Tue, 1 Oct 2024 at 15:52, Daniel Gustafsson  wrote:
> > So we need to think about a way to make this more robust for future people 
> > editing.  Maybe something in .gitattributes or some editor settings.  
> > Otherwise, it will be all over the places after a while.
>
> Maybe we can add some form of pandoc target for rendering as as way to test
> locally before pushing?

I think a gitattributes rule to disallow hard-tabs word work fine,
especially when combined with this patch of mine which keeps the
.editorconfig file in sync with the .gitattributes file:
https://commitfest.postgresql.org/49/4829/

> > Apart from this, I don't changing the placeholders like  to < foo >.  
> > In some cases, this really decreases readability.  Maybe we should look for 
> > different approaches there.
>
> Agreed.  I took a stab at some of them in the attached.  The usage in
> src/test/isolation/README is seemingly the hardest to replace and I'm not sure
> how we should proceed there.

One way to improve the isolation/README situation is by:
1. indenting the standalone lines by four spaces to make it a code block
2. for the inline cases, replace  with `` or `foo`




Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-09-26 Thread Jelte Fennema-Nio
On Thu, 26 Sept 2024 at 08:06, Robert Haas  wrote:
> Focusing on the first patch seems odd to me, though

Indeed the first few patches will often be small, and the big patch
will appear later. When I split patches up, those small patches should
usually be reviewable without looking at the big patch in detail, and
hopefully they shouldn't be too contentious: e.g. a comment
improvement or some small refactor. But often those patches don't seem
to be reviewed significantly quicker or merged significantly earlier
than the big patch. That makes it seem to me that even though they
should be relatively low-risk to commit and low-effort to review,
reviewers are scared away by the sheer number of patches in the
patchset, or by the size of the final patch. That's why I thought it
could be useful to specifically show the size of the first patch in
addition to the total patchset size, so that reviewers can easily spot
some small hopefully easy to review patch at the start of a patchset.




Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-09-25 Thread Jelte Fennema-Nio
On Tue, 24 Sept 2024 at 23:54, Jelte Fennema-Nio  wrote:
> A bunch of other improvements also got deployed:
> - Improved homepage[1] (now with useful and bookmarkable links at the top)
> - More links on the cf entry page[2] (cfbot results, github diff, and

[1]: https://commitfest.postgresql.org/
[2]: https://commitfest.postgresql.org/patch/5070




Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-09-24 Thread Jelte Fennema-Nio
On Fri, 16 Aug 2024 at 07:43, Tom Lane  wrote:
> However, there are other ways to accomplish that.  I liked the
> suggestion of extending the CF webapp with a way to search for entries
> mentioning a particular mail message ID.  I dunno how hard it'd be to
> get it to recognize *any* message-ID from a thread, but surely at
> least the head message ought not be too hard to match.

This is now deployed, so you can now find a CF entry based on the email ID.

A bunch of other improvements also got deployed:
- Improved homepage[1] (now with useful and bookmarkable links at the top)
- More links on the cf entry page[2] (cfbot results, github diff, and
stable link to entry itself)
- Instructions on how to checkout an cfbot entry

CFBot traffic lights directly on the cfentry and probably the
commifest list page are the next thing I'm planning to work on

After that I'll take a look at sending opt-in emails

Another thing that I'm interested in adding is some metric of patch
size, so it's easier to find small patches that are thus hopefully
"easy" to review. To accommodate multi-patch emails, I'm thinking of
showing lines changed in the first patch and lines changed in all
patches together. Possibly showing it clearly, if significantly more
lines were deleted than added, so it's easy to spot effective
refactorings.




Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bind

2024-09-19 Thread Jelte Fennema-Nio
On Thu, 19 Sept 2024 at 09:30, Michael Paquier  wrote:
> Issuing an error if there is a state does not sound like a good idea
> at this stage because it would suddenly break scripts that expect
> multiple commands of \bind to prioritize the last one.

Seems like a good idea to add a simple test for that behaviour then.
See attached.

On Thu, 19 Sept 2024 at 09:30, Michael Paquier  wrote:
>
> On Wed, Sep 18, 2024 at 06:08:54PM +0900, Michael Paquier wrote:
> > On Wed, Sep 18, 2024 at 09:42:43AM +0200, Anthonin Bonnefoy wrote:
> >> I've joined a patch to clean the psql extended state at the start of
> >> every extended protocol backslash command, freeing the allocated
> >> variables and resetting the send_mode. Another possible approach would
> >> be to return an error when there's already an existing state instead
> >> of overwriting it.
> >
> > I'll double-check all that tomorrow, but you have looks like it is
> > going in the right direction.
>
> And done down to v16, with one logic for HEAD and something simpler
> for \bind in v16 and v17.
>
> Issuing an error if there is a state does not sound like a good idea
> at this stage because it would suddenly break scripts that expect
> multiple commands of \bind to prioritize the last one.  If that was
> something only on HEAD, I would have considered that as a serious
> option, but not with v16 in mind for \bind.
> --
> Michael
From 20637d0fdfb08ed7e79a241b82b1d4f9f9f34d8c Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio 
Date: Thu, 19 Sep 2024 10:50:53 +0200
Subject: [PATCH v1] psql: Add test for repeated \bind calls

---
 src/test/regress/expected/psql.out | 19 +++
 src/test/regress/sql/psql.sql  |  5 +
 2 files changed, 24 insertions(+)

diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index cf040fbd80..83a51c9682 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -173,6 +173,25 @@ SELECT $1, $2 \bind 'foo' 'bar' \g
  foo  | bar
 (1 row)
 
+-- second bind overwrites first
+select $1 as col \bind 1 \bind 2 \g
+ col 
+-
+ 2
+(1 row)
+
+-- unless there's a \g in between
+select $1 as col \bind 1 \g \bind 2 \g
+ col 
+-
+ 1
+(1 row)
+
+ col 
+-
+ 2
+(1 row)
+
 -- errors
 -- parse error
 SELECT foo \bind \g
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 8de90c805c..b027e0eb7d 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -76,6 +76,11 @@ SELECT 1 \bind \g
 SELECT $1 \bind 'foo' \g
 SELECT $1, $2 \bind 'foo' 'bar' \g
 
+-- second bind overwrites first
+select $1 as col \bind 1 \bind 2 \g
+-- unless there's a \g in between
+select $1 as col \bind 1 \g \bind 2 \g
+
 -- errors
 -- parse error
 SELECT foo \bind \g
-- 
2.43.0



Re: Detailed release notes

2024-09-18 Thread Jelte Fennema-Nio
On Wed, 18 Sept 2024 at 13:38, Marcos Pegoraro  wrote:
>> Maybe this shouldn't be done between RC1 and GA.  This is clearly a more
>> complex topic that should go through a proper review and testing cycle.
>
> I think this is just a question of decision, not reviewing or testing.
> And I'm sure there will be more people reading Release Notes now, in 
> September and October, than in January or April.

+1. Also, IMHO the links even in their current simple form of "§ § §
§" are a huge net-positive on the release notes. So yes, I'm sure we
can format them more clearly with some bikeshedding, but removing them
completely seems like an overreaction to me.




Re: Detailed release notes

2024-09-18 Thread Jelte Fennema-Nio
On Wed, 18 Sept 2024 at 11:26, Alvaro Herrera  wrote:
>
> On 2024-Sep-17, Bruce Momjian wrote:
>
> > I think trying to add text to each item is both redundant and confusing,
> > because I am guessing that many people will not even know what a commit
> > is, and will be confused by clicking on the link.
>
> Uh, I 100% disagree that Postgres users reading the release notes would
> not know what a commit is.

+1, IMO it seems completely reasonable to assume that anyone
interested enough in postgres to read the release notes also knows
what a commit is.

> IMO we should be looking at a more surgical approach to implement this,
> perhaps using a custom SGML tag and some custom XSLT code to process
> such tags that adds links the way we want, rather than generic 
> tags.  Or maybe  is OK if we add some class to it that XSLT knows
> to process differently than generic ulinks, like func_table_entry and
> catalog_table_entry.

Is it easy to just remove/hide the links completely from the PDF? I
doubt many people read the release notes by going to Appendix E in the
PDF anyway.

It seems a shame to remove those links from the HTML view where they
look acceptable and which most people will use, just because they look
bad in the pdf. And honestly, they don't even look that terrible in
the PDF imo.




Re: Detailed release notes

2024-09-18 Thread Jelte Fennema-Nio
On Wed, 18 Sept 2024 at 02:55, Bruce Momjian  wrote:
> > Also very clutter-y.  I'm not convinced that any of this is a good
> > idea that will stand the test of time: I estimate that approximately
> > 0.01% of people who read the release notes will want these links.
>
> Yes, I think 0.01% is accurate.

I think that is a severe underestimation. People that read the release
notes obviously read it because they want to know what changed. But
the release notes are very terse (on purpose, and with good reason),
and people likely want a bit more details on some of the items that
they are particularly interested in. These links allow people to
easily find details on those items. They might not care about the
actual code from the commit, but the commit message is very likely
useful to them.




Re: Detailed release notes

2024-09-17 Thread Jelte Fennema-Nio
On Tue, 17 Sept 2024 at 10:12, Alvaro Herrera  wrote:
> Maybe it would work to use numbers in square brackets instead:
>
> Add backend support for injection points (Michael Paquier) [1] [2] [3] [4]

Another option would be to add them in superscript using the 
html tag (or even using some unicode stuff):

Add backend support for injection points (Michael Paquier)1,
2, 3, 4

So similar to footnotes in a sense, i.e. if you want details click on
the small numbers




Re: First draft of PG 17 release notes

2024-09-17 Thread Jelte Fennema-Nio
On Wed, 11 Sept 2024 at 16:10, Bruce Momjian  wrote:
> You are right that I do mention changes specifically designed for the
> use of extensions, but there is no mention in the commit message of its
> use for extensions.  In fact, I thought this was too low-level to be of
> use for extensions.  However, if people feel it should be added, we have
> enough time to add it.

Another new API that is useful for extension authors is the following
one (I'm obviously biased since I'm the author, and I don't know if
there's still time):

commit 14dd0f27d7cd56ffae9ecdbe324965073d01a9ff
Author: Nathan Bossart 
Date:   Thu Jan 4 16:09:34 2024 -0600

Add macros for looping through a List without a ListCell.

Many foreach loops only use the ListCell pointer to retrieve the
content of the cell, like so:

ListCell   *lc;

foreach(lc, mylist)
{
int myint = lfirst_int(lc);

...
}

This commit adds a few convenience macros that automatically
declare the loop variable and retrieve the current cell's contents.
This allows us to rewrite the previous loop like this:

foreach_int(myint, mylist)
{
...
}

> An interesting idea would be
> to report all function signature changes in each major release in some
> way.

I think that might be useful, but it very much depends how long that
list gets. If it gets too long I think authors will just try to
compile and only look at the ones that break for them.




Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-09-12 Thread Jelte Fennema-Nio
On Thu, 12 Sept 2024 at 04:07, David Rowley  wrote:
> Maybe instead of the URLs showing the CF
> number that the patch was first added to, if it could have a reference
> in the URL that looks up the maximum CF which contains the patch and
> show that one. e.g. https://commitfest.postgresql.org/latest/4751/ .

Yeah agreed, that's why I added support for
https://commitfest.postgresql.org/patch/4751/ a while ago. That URL
will be easily discoverable on the commitfest entry page soon (patch
for this is in flight).




Re: First draft of PG 17 release notes

2024-09-09 Thread Jelte Fennema-Nio
On Tue, 10 Sept 2024 at 04:47, Bruce Momjian  wrote:
> Yes.  There are so many changes at the source code level it is unwise to
> try and get them into the main release notes.  If someone wants to
> create an addendum, like was suggested for pure performance
> improvements, that would make sense.

I agree that the release notes cannot fit every change. But I also
don't think any extension author reads the complete git commit log
every release, so taking the stance that they should be seems
unhelpful. And the "Source Code" section does exist so at some level
you seem to disagree with that too. So what is the way to decide that
something makes the cut for the "Source Code" section?

I think as an extension author there are usually three types of
changes that are relevant:
1. New APIs/hooks that are meant for extension authors
2. Stuff that causes my existing code to not compile anymore
3. Stuff that changes behaviour of existing APIs code in a
incompatible but silent way

For 1, I think adding them to the release notes makes total sense,
especially if the new APIs are documented not only in source code, but
also on the website. Nathan his change is of this type, so I agree
with him it should be in the release notes.

For 2, I'll be able to easily find the PG commit that caused the
compilation failure by grepping git history for the old API. So having
these changes in the release notes seems unnecessary.

For 3, it would be very useful if it would be in the release notes,
but I think in many cases it's hard to know what commits do this. So
unless it's obviously going to break a bunch of extensions silently, I
think we don't have to add such changes to the release notes.




Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-09-09 Thread Jelte Fennema-Nio
On Tue, Sep 10, 2024, 00:52 Jelte Fennema-Nio  wrote:

> On Mon, Sep 9, 2024, 22:02 Robert Haas  wrote:
>
>> Should I #blamemagnus?
>>
>
> Yes. He seems unavailable for whatever reason based on my private holidays
> (maybe summer holidays)
>

*based on my private/off-list communication with him

>


Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-09-09 Thread Jelte Fennema-Nio
On Mon, Sep 9, 2024, 22:02 Robert Haas  wrote:

> Should I #blamemagnus?
>

Yes. He seems unavailable for whatever reason based on my private holidays
(maybe summer holidays)

>


Re: PostgreSQL 17 release announcement draft

2024-09-06 Thread Jelte Fennema-Nio
On Fri, 6 Sept 2024 at 19:04, Jonathan S. Katz  wrote:
> Please see v2 attached. As per original note, please provide feedback
> before Mon, Sep 9 @ 12:00 UTC so we can begin the translation process.

The following sentence was part of the beta1 release announcement, but
is not part of this draft. Did that happen by accident or was it done
on purpose? If on purpose, I'm curious why since I contributed the
feature.

"PostgreSQL 17 also provides better support for asynchronous and more
secure query cancellation routines, which drivers can adopt using the
libpq API."




Re: Detailed release notes

2024-09-06 Thread Jelte Fennema-Nio
On Thu, 22 Aug 2024 at 21:34, Marcos Pegoraro  wrote:
> I understand your point, and agree with that for previous releases, but since 
> we have a month only for version 17, will this process work properly until 
> that date ?
> I think a release notes is more read as soon as it is available than other 
> months, isn't it ?
> And this feature is just a HTML page, so if it's done manually or 
> automatically, from the reader point of view it'll be exactly the same.

Big +1 to this. It would definitely be great if we would have these
commit links for previous release notes. But the PG17 GA release is
probably happening in 19 days on September 26th. I feel like we should
focus on getting the manual improvements from Jian He merged,
otherwise we'll end up with release notes for PG17 on release day that
are significantly less useful than they could have been. Let's not
make perfect the enemy of good here.




Re: Add trim_trailing_whitespace to editorconfig file

2024-09-05 Thread Jelte Fennema-Nio
On Fri, 9 Aug 2024 at 16:09, Jelte Fennema-Nio  wrote:
> That's there so that the generated .editorconfig file the correct
> indent_size. I guess another approach would be to change the
> generate_editorconfig.py script to include hardcoded values for these
> 4 filetypes.

Okay, I've done this now. Any chance this can "just" be committed now?
Having to remove trailing spaces by hand whenever I edit sgml files is a
continuous annoyance to me when working on other patches.


v8-0001-Add-script-to-keep-.editorconfig-in-sync-with-.gi.patch
Description: Binary data


Re: Create syscaches for pg_extension

2024-09-05 Thread Jelte Fennema-Nio
On Thu, 5 Sept 2024 at 22:03, Andrei Lepikhov  wrote:
> Thanks, see new patch in attachment.

LGTM now. Added it to the commitfest here:
https://commitfest.postgresql.org/50/5241/




Re: Create syscaches for pg_extension

2024-09-05 Thread Jelte Fennema-Nio
On Thu, 5 Sept 2024 at 15:41, Andrei Lepikhov  wrote:
> It seems that the get_extension_oid routine was not modified when the
> sys cache was introduced. What is the reason? It may be that this
> routine is redundant now, but if not, and we want to hold the API that
> extensions use, maybe we should rewrite it, too.
> See the attachment proposing changes.

It seems reasonable to make this function use the new syscache. I
didn't change any existing code in my original patch, because I wanted
to use the syscache APIs directly anyway and I didn't want to make the
patch bigger than strictly necessary. But I totally understand that
for many usages it's probably enough if the existing APIs are simply
faster (on repeated calls). The patch looks fine. But I think
get_extension_name and get_extension_schema should also be updated.




Re: Make query cancellation keys longer

2024-09-05 Thread Jelte Fennema-Nio
On Thu, 5 Sept 2024 at 17:43, Jacob Champion
 wrote:
> Has there been any work/discussion around not sending the cancel key
> in plaintext from psql? It's not a prerequisite or anything (the
> longer length is a clear improvement either way), but it seems odd
> that this longer "secret" is still just going to be exposed on the
> wire when you press Ctrl+C.

Totally agreed that it would be good to update psql to use the new
much more secure libpq function introduced in PG17[1]. This is not a
trivial change though because it requires refactoring the way we
handle signals (which is why I didn't do it as part of introducing
these new APIs). I had hoped that the work in [2] would either do that
or at least make it a lot easier, but that thread seems to have
stalled. So +1 for doing this, but I think it's a totally separate
change and so should be discussed on a separate thread.

[1]: https://www.postgresql.org/docs/17/libpq-cancel.html#LIBPQ-CANCEL-FUNCTIONS
[2]: 
https://www.postgresql.org/message-id/flat/20240331222502.03b5354bc6356bc5c388919d%40sraoss.co.jp#1450c8fee45408acaa5b5a1b9a6f70fc

> For the cancel key implementation in particular, I agree with you that
> it's probably not a serious problem. But if other security code starts
> using timingsafe_bcmp() then it might be something to be concerned
> about. Are there any platform/architecture combos that don't provide a
> native timingsafe_bcmp() *and* need a DIT bit for safety?

It sounds to me like we should at least use OpenSSL's CRYPTO_memcmp if
we linked against it and the OS doesn't provide a timingsafe_bcmp.
Would that remove your concerns? I expect anyone that cares about
security to link against some TLS library. That way our "fallback"
implementation is only used on the rare systems where that's not the
case.




Re: [EXTERNAL] Re: Add non-blocking version of PQcancel

2024-08-31 Thread Jelte Fennema-Nio
On Sat, 31 Aug 2024 at 06:04, Alexander Lakhin  wrote:
> At the same time, mylodon confirmed my other finding at [1] and failed [2] 
> with:
> -ERROR:  canceling statement due to statement timeout
> +ERROR:  canceling statement due to user request
>
> [1] 
> https://www.postgresql.org/message-id/4db099c8-4a52-3cc4-e970-14539a319466%40gmail.com
> [2] 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mylodon&dt=2024-08-30%2023%3A03%3A31

Interestingly that's a different test that failed, but it looks like
it failed for the same reason that my 0002 patch fixes.

I also took a quick look at the code again, and completely removing
the outstanding interrupt seems hard to do. Because there's no way to
know if there were multiple causes for the interupt, i.e. someone
could have pressed ctrl+c as well and we wouldn't want to undo that.

So I think the solution in 0002, while debatable if strictly correct,
is the only fix that we can easily do. Also I personally believe the
behaviour resulting from 0002 is totally correct: The new behaviour
would be that if a timeout occurred, right before it was disabled or
reset, but the interrupt was not processed yet, then we process that
timeout as normal. That seems totally reasonable behaviour to me from
the perspective of an end user: You get a timeout error when the
timeout occurred before the timeout was disabled/reset.




Re: [EXTERNAL] Re: Add non-blocking version of PQcancel

2024-08-30 Thread Jelte Fennema-Nio
On Fri, 30 Aug 2024 at 22:12, Tom Lane  wrote:
>
> Jelte Fennema-Nio  writes:
> > On Fri, Aug 30, 2024, 21:21 Tom Lane  wrote:
> >> While we're piling on, has anyone noticed that *non* Windows buildfarm
> >> animals are also failing this test pretty frequently?
>
> > Yes. Fixes are here (see the ~10 emails above in the thread for details):
> > https://www.postgresql.org/message-id/cageczqqo8cn2rw45xuymvzxet7-bcruuzufmbgqc3ue...@mail.gmail.com
>
> Hmm.  I'm not convinced that 0001 is an actual *fix*, but it should
> at least reduce the frequency of occurrence a lot, which'd help.

I also don't think it's an actual fix, but I couldn't think of a way
to fix this. And since this only happens if you cancel right at the
start of a postgres_fdw query, I don't think it's worth investing too
much time on a fix.

> I don't want to move the test case to where you propose, because
> that's basically not sensible.  But can't we avoid remote estimates
> by just cross-joining ft1 to itself, and not using the tables for
> which remote estimate is enabled?

Yeah that should work too (I just saw your next email, where you said
it's committed like this).

> I think 0002 is probably outright wrong, or at least the change to
> disable_statement_timeout is.  Once we get to that, we don't want
> to throw a timeout error any more, even if an interrupt was received
> just before it.

The disable_statement_timeout change was not the part of that patch
that was necessary for stable output, only the change in the first
branch of enable_statement_timeout was necessary. The reason being
that enable_statement_timeout is called multiple times for a query,
because start_xact_command is called multiple times in
exec_simple_query. The change to disable_statement_timeout just seemed
like the logical extension of that change, especially since there was
basically a verbatim copy of disable_statement_timeout in the second
branch of enable_statement_timeout.

To make sure I understand your suggestion correctly: Are you saying
you would want to completely remove the outstanding interrupt if it
was caused by de statement_timout when disable_statement_timeout is
called? Because I agree that would probably make sense, but that
sounds like a more impactful change. But the current behaviour seems
strictly worse than the behaviour proposed in the patch to me, because
currently the backend would still be interrupted, but the error would
indicate a reason for the interrupt that is simply incorrect i.e. it
will say it was cancelled due to a user request, which never happened.




Re: [EXTERNAL] Re: Add non-blocking version of PQcancel

2024-08-30 Thread Jelte Fennema-Nio
On Fri, Aug 30, 2024, 21:21 Tom Lane  wrote:

>
> While we're piling on, has anyone noticed that *non* Windows buildfarm
> animals are also failing this test pretty frequently?
>


Any thoughts?
>

Yes. Fixes are here (see the ~10 emails above in the thread for details):
https://www.postgresql.org/message-id/cageczqqo8cn2rw45xuymvzxet7-bcruuzufmbgqc3ue...@mail.gmail.com

They don't apply anymore after the change to move this test to a dedicated
file. It shouldn't be too hard to update those patches though. I'll try to
do that in a few weeks when I'm back behind my computer. But feel free to
commit something earlier.

>


Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

2024-08-23 Thread Jelte Fennema-Nio
On Fri, 23 Aug 2024 at 16:02, Robert Haas  wrote:
> Personally, I'm 100% convinced at this point that we're arguing about
> the wrong problem. Before, I didn't know for sure whether anyone would
> be mad if we redefined PQprotocolVersion(), but now I know that there
> is at least one person who will be, and that's Jacob.

I could be interpreting Jacob his response incorrectly, but my
understanding is that the type of protocol changes we would actually
do in this version bump, would determine if he's either mad or happy
that we redefined PQprotocolVersion.

> If there's one
> among regular -hackers posters, there are probably more. Since Jelte
> doesn't seem to want to produce the patch to add
> PQminorProtocolVersion(), I suggest that somebody else does that --
> Jacob, do you want to? -- and we commit that and move on.

Let's call it PQfullProtocolVersion and make it return 30002. I'm fine
with updating the patch. But I'll be unavailable for the next ~3
weeks.

> Then we can get down to the business of actually changing some stuff
> at the protocol level. IMHO, that's what should be scary and/or
> controversial here, and it's also imperative that if we're going to do
> it, we do it soon.

Agreed, but I don't think doing so is blocked on merging a
PQfullProtocolVersion libpq function.




Re: On disable_cost

2024-08-22 Thread Jelte Fennema-Nio
On Wed, 31 Jul 2024 at 18:23, Robert Haas  wrote:
> - If we do commit 0002, I think it's a good idea to have the number of
> disabled nodes displayed even with COSTS OFF, because it's stable, and
> it's pretty useful to be able to see this in the regression output. I
> have found while working on this that I often need to adjust the .sql
> files to say EXPLAIN (COSTS ON) instead of EXPLAIN (COSTS OFF) in
> order to understand what's happening. Right now, there's no real
> alternative because costs aren't stable, but disabled-node counts
> should be stable, so I feel this would be a step forward. Apart from
> that, I also think it's good for features to have regression test
> coverage, and since we use COSTS OFF everywhere or at least nearly
> everywhere in the regression test, if we don't print out the disabled
> node counts when COSTS OFF is used, then we don't cover that case in
> our tests. Bummer.

Are the disabled node counts still expected to be stable even with
GEQO? If not, maybe we should have a way to turn them off after all.
Although I agree that always disabling them when COSTS OFF is set is
probably also undesirable. How about a new option, e.g. EXPLAIN
(DISABLED OFF)




Re: RFC: Additional Directory for Extensions

2024-08-22 Thread Jelte Fennema-Nio
On Thu, 22 Aug 2024 at 01:08, Craig Ringer
 wrote:
> SET extension_search_path = $extsdir,
> /mnt/extensions/pg16/postgis-vX.Y/extensions,
> /mnt/extensions/pg16/gosuperfast/extensions;

It looks like you want one directory per extension, so that list would
get pretty long if you have multiple extensions. Maybe (as a follow up
change), we should start to support a * as a wildcard in both of these
GUCs. So you could say:

SET extension_search_path = /mnt/extensions/pg16/*

To mean effectively the same as you're describing above.




Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

2024-08-20 Thread Jelte Fennema-Nio
On Tue, 20 Aug 2024 at 23:48, Jacob Champion
 wrote:
> That guarantee (if adopted) would also make it possible for my use
> case to proceed correctly, since a libpq client can still speak 3.0
> packets on the socket safely.

Not necessarily (at least not how I defined it). If a protocol
parameter has been configured (possibly done by default by libpq),
then that might not be the case anymore. So, you'd also need to
compare the current values of the protocol parameters to their
expected value.

> But in that case, PQprotocolVersion
> should keep returning 3, because there's an explicit reason to care
> about the major version by itself.

I agree that there's a reason to care about the major version then,
but it might still be better to fail hard for people that care about
protocol details. Because when writing the code that checked
PQprotocolVersion there was no definition at all of what could change
during a minor version bump. So there was no possibility to reason for
that person if they should be notified of a minor version bump or not.
So notifying the author of the code by failing the check would be
erring on the safe side, because maybe they would need to update their
code that depends on the protocol details. If not, and they then
realize that our guarantee is strong enough for their use case, then
they could replace their check with something like:

PQprotocolVersion() >= 3 && PQprotocolVersion() < 4

To be clear, the argument for changing PQprotocolVersion() is
definitely less strong if we'd provide such a guarantee. But I don't
think the problem is completely gone either.




Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bind

2024-08-20 Thread Jelte Fennema-Nio
On Thu, 25 Jul 2024 at 08:45, Anthonin Bonnefoy
 wrote:
> +1 keeping this as a separate command and using \bind_named. \bind has
> a different behaviour as it also parses the query so keeping them as
> separate commands would probably avoid some confusion.

+1 on naming it \bind_named

@Anthonin are you planning to update the patch accordingly?




Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

2024-08-20 Thread Jelte Fennema-Nio
On Tue, 20 Aug 2024 at 19:31, Jacob Champion
 wrote:
> It's applicable to the use case I was talking about with Jelte. A
> libpq client dropping down to the socket level is relying on
> (implicit, currently undocumented/undecided, possibly incorrect!)
> intermediary guarantees that the protocol provides for a major
> version. I'm hoping we can provide some, since we haven't broken
> anything yet. If we decide we can't, then so be it -- things will
> break either way -- but it's still strange to me that we'd be okay
> with literally zero forward compatibility and still call that a "minor
> version".

I think one compatibility guarantee that we might want to uphold is
something like the following: After completing the initial connection
setup, a server should only send new message types or new fields on
existing message types when the client has specifically advertised
support for that message type in one of two ways:
1. By configuring a specific protocol parameter
2. By sending a new message type or using a new field that implicitly
advertises support for the new message type/fields. In this case the
message should be of a request-response style, the server cannot
assume that after the request-response communication happened this new
message type is still supported by the client.

The reasoning for this was discussed a while back upthread: This would
be to allow a connection pooler (like PgBouncer) to have a pool of the
highest minor version that the pooler supports e.g 3.8, but then it
could still hand out these connections to clients that connected to
the pooler using a lower version. Without having these clients receive
messages that they do not support.

Another way of describing this guarantee: If a client connects using
3.8 and configures no protocol parameters, the client needs to handle
anything 3.8 specific that the handshake requires (such as longer
cancel token). But then after that handshake it can choose to send
only 3.0 packets and expect to receive only 3.0 packets back.




Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

2024-08-20 Thread Jelte Fennema-Nio
On Tue, 20 Aug 2024 at 19:02, David G. Johnston
 wrote:
> So basically my proposal amounted to making every update a "major version 
> update" and changing the behavior surrounding NegotiateProtocolVersion so it 
> applies to major version differences.  I'll stand by that change in 
> definition.  The current one doesn't seem all that useful anyway, and as we 
> only have a single version, definitely hasn't been materially implemented.  
> Otherwise, at some point a client that knows both v3 and v4 will exist and 
> its connection will be rejected instead of downgraded by a v3-only server 
> even though such a downgrade would be possible.  I suspect we'd go ahead and 
> change the rule then - so why not just do so now, while getting rid of the 
> idea that minor versions are a thing.

If we decide to never change the format of the StartupMessage again
(which may be an okay thing to decide). Then I agree it would make
sense to update the existing supported servers ASAP to be able to send
back a NegotiateProtocolVersion message if they receive a 4.x
StartupMessage, and the server only supports up to 3.x.

However, even if we do that, I don't think it makes sense to start
using the 4.0 version straight away. Because many older postgres
servers would still throw an error when receiving the 4.x request. By
using a 3.x version we are able to avoid those errors in the existing
ecosystem. Basically, I think we should probably wait ~5 years again
until we actually use a 4.0 version.

i.e. I don't see serious benefits to using 4.0.  The main benefit you
seem to describe is: "it's theoretically cleaner to use major version
bumps". And there is a serious downside: "seriously breaking the
existing ecosystem".

> I suppose we could leave minor versions for patch releases of the main server 
> version - which still leaves the first new feature of a release incrementing 
> the major version.  That would be incidental to changing how we handle major 
> versions.

Having a Postgres server patch update change the protocol version that
the server supports sounds pretty scary to me.




Re: Partial aggregates pushdown

2024-08-20 Thread Jelte Fennema-Nio
On Tue, 20 Aug 2024 at 18:50, Bruce Momjian  wrote:
> Okay, so we can do MAX easily, and AVG if the count can be represented
> as the same data type as the sum?  Is that correct?  Our only problem is
> that something like AVG(interval) can't use an array because arrays have
> to have the same data type for all array elements, and an interval can't
> represent a count?

Close, but still not completely correct. AVG(bigint) can also not be
supported by patch 1, because the sum and the count for that both
stored using an int128. So we'd need an array of int128, and there's
currently no int128 SQL type.




Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

2024-08-20 Thread Jelte Fennema-Nio
On Tue, 20 Aug 2024 at 18:42, David G. Johnston
 wrote:
> v18 libpq-based clients, if they attempt to connect using v4 and fail, will 
> try again using the v3 connection.  That will retain status quo behavior when 
> something like a connection pooler doesn't understand the new reality.

Having connection latency double when connecting to an older Postgres
is something I'd very much like to avoid. Reconnecting functionally
retains the status quo, but it doesn't retain the expected perf
characteristics. By using a minor protocol version we can easily avoid
this connection latency issue.




Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

2024-08-20 Thread Jelte Fennema-Nio
On Tue, 20 Aug 2024 at 17:46, Robert Haas  wrote:
> I personally like this less than both (a) adding a new function and
> (b) redefining the existing function as Jelte proposes. It just seems
> too clever to me.

Agreed, I'm not really seeing a benefit of returning 4 instead of
30004. Both are new numbers that are higher than 3, so on existing
code they would have the same impact. But any new code would be more
readable when using version >= 30004 imho.




Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

2024-08-20 Thread Jelte Fennema-Nio
On Tue, 20 Aug 2024 at 15:48, Jacob Champion
 wrote:
> Put another way: for a middlebox on the connection (which may be
> passively observing, but also maybe actively adding new messages to
> the stream), what is guaranteed to remain the same in the protocol
> across a minor version bump? Hopefully the answer isn't "nothing"?

I think primarily we do a minor version bump because a major version
bump would cause existing Postgres servers to throw an error for the
connection attempt (and we don't want that). While for a minor version
bump they will instead send a NegotiateProtocolVersion message.

In practical terms I think that means for a minor version bump the
format of the StartupMessage cannot be changed. Changing anything else
is fair game for a minor protocol version bump. I think we probably
would not want to change the format of ErrorResponse and
NoticeResponse, since those can be sent by the server before the
NegotiateProtocolVersion message. But I don't even think that is
strictly necessary, as long as clients would be able to parse both the
old and new versions.

Note that this definition arises from code and behaviour introduced in
ae65f6066dc3 in 2017. And PQprotocolVersion was introduced in
efc3a25bb0 in 2003. So anyone starting to use the PQprotocolVersion
function in between 2003 and 2017 had no way of knowing that there
would ever be a thing called a "minor" version, in which anything
about the protocol could be changed except for the StartupMessage.




Re: Partial aggregates pushdown

2024-08-20 Thread Jelte Fennema-Nio
On Thu, 15 Aug 2024 at 23:12, Bruce Momjian  wrote:
> Third, I would like to show a more specific example to clarify what is
> being considered above.  If we look at MAX(), we can have FDWs return
> the max for each FDW, and the coordinator can chose the highest value.
> This is the patch 1 listed above.  These can return the
> pg_aggregate.aggtranstype data type using the pg_type.typoutput text
> output.
>
> The second case is for something like AVG(), which must return the SUM()
> and COUNT(), and we currently have no way to return multiple text values
> on the wire.  For patch 0002, we have the option of creating functions
> that can do this and record them in new pg_attribute columns, or we can
> create a data type with these functions, and assign the data type to
> pg_aggregate.aggtranstype.
>
> Is that accurate?

It's close to accurate, but not entirely. Patch 1 would actually
solves some AVG cases too, because some AVG implementations use an SQL
array type to store the transtype instead of an internal type. And by
using an SQL array type we *can* send multiple text values on the
wire. See below for a list of those aggregates:

> select p.oid::regprocedure
from pg_aggregate a join pg_proc p on a.aggfnoid = p.oid
where aggfinalfn != 0 and aggtranstype::regtype not in ('internal',
'anyenum', 'anyelement', 'anyrange', 'anyarray', 'anymultirange');
oid
───
 avg(integer)
 avg(smallint)
 avg(real)
 avg(double precision)
 avg(interval)
 var_pop(real)
 var_pop(double precision)
 var_samp(real)
 var_samp(double precision)
 variance(real)
 variance(double precision)
 stddev_pop(real)
 stddev_pop(double precision)
 stddev_samp(real)
 stddev_samp(double precision)
 stddev(real)
 stddev(double precision)
 regr_sxx(double precision,double precision)
 regr_syy(double precision,double precision)
 regr_sxy(double precision,double precision)
 regr_avgx(double precision,double precision)
 regr_avgy(double precision,double precision)
 regr_r2(double precision,double precision)
 regr_slope(double precision,double precision)
 regr_intercept(double precision,double precision)
 covar_pop(double precision,double precision)
 covar_samp(double precision,double precision)
 corr(double precision,double precision)
(28 rows)

And to be clear, these are in addition to the MAX type of aggregates
you were describing:
> select p.oid::regprocedure
from pg_aggregate a join pg_proc p on a.aggfnoid = p.oid
where aggfinalfn = 0 and aggtranstype::regtype not in ('internal',
'anyenum', 'anyelement', 'anyrange', 'anyarray', 'anymultirange');
  oid
───
 sum(integer)
 sum(smallint)
 sum(real)
 sum(double precision)
 sum(money)
 sum(interval)
 max(bigint)
 max(integer)
 max(smallint)
 max(oid)
 max(real)
 max(double precision)
 max(date)
 max(time without time zone)
 max(time with time zone)
 max(money)
 max(timestamp without time zone)
 max(timestamp with time zone)
 max(interval)
 max(text)
 max(numeric)
 max(character)
 max(tid)
 max(inet)
 max(pg_lsn)
 max(xid8)
 min(bigint)
 min(integer)
 min(smallint)
 min(oid)
 min(real)
 min(double precision)
 min(date)
 min(time without time zone)
 min(time with time zone)
 min(money)
 min(timestamp without time zone)
 min(timestamp with time zone)
 min(interval)
 min(text)
 min(numeric)
 min(character)
 min(tid)
 min(inet)
 min(pg_lsn)
 min(xid8)
 count("any")
 count()
 regr_count(double precision,double precision)
 bool_and(boolean)
 bool_or(boolean)
 every(boolean)
 bit_and(smallint)
 bit_or(smallint)
 bit_xor(smallint)
 bit_and(integer)
 bit_or(integer)
 bit_xor(integer)
 bit_and(bigint)
 bit_or(bigint)
 bit_xor(bigint)
 bit_and(bit)
 bit_or(bit)
 bit_xor(bit)
 xmlagg(xml)
(65 rows)




Re: ANALYZE ONLY

2024-08-20 Thread Jelte Fennema-Nio
On Tue, 20 Aug 2024 at 07:52, Michael Harris  wrote:
>   1. Would such a feature be welcomed? Are there any traps I might not
> have thought of?

I think this sounds like a reasonable feature.


>   2. The existing ANALYZE command has the following structure:
>
>  ANALYZE [ ( option [, ...] ) ] [ table_and_columns [, ...] ]
>
>  It would be easiest to add ONLY as another option, but that
> doesn't look quite
>  right to me - surely the ONLY should be attached to the table name?
>  An alternative would be:
>
>  ANALYZE [ ( option [, ...] ) ] [ONLY] [ table_and_columns [, ...] ]
>
> Any feedback or advice would be great.

Personally I'd prefer a new option to be added. But I agree ONLY isn't
a good name then. Maybe something like SKIP_PARTITIONS.




Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

2024-08-19 Thread Jelte Fennema-Nio
On Mon, 19 Aug 2024 at 16:16, Robert Haas  wrote:
> If somebody is using PQprotocolVersion() to detect the arrival of a
> new protocol version, it stands to reason that they only care about
> new major protocol versions, because that's what the function is
> defined to tell you about. Anyone who has done a moderate amount of
> looking into this area will understand that the protocol has a major
> version number and a minor version number and that this function only
> returns the former. Therefore, they should expect that the arrival of
> a new minor protocol version won't change the return value of this
> function.

What I'm trying to say is: I don't think there's any usecase where
people would care about a major bump, but not a minor bump. Especially
keeping in mind that a minor bump had never occurred when originally
creating this function. And because we never did it, there has so far
been no definition of what is the actual difference between a major
and a minor bump.

> I really don't understand why we're still arguing about this. It seems
> to me that we've established that there is some usage of the existing
> function, and that changing the return value will break something.
> Sure, so far as we know that something is "only" regression tests, but
> there's no guarantee that there couldn't be other code that we don't
> know about that breaks worse

My point is that the code that breaks, actually wants to be broken in this case.

> and even there isn't, who wants to break
> regression tests when there's nothing actually wrong?

Updating the regression test would be less work than adding support
for a new API. So if the main problem is

> Now we could
> decide we're going to do it anyway because of whatever reason we might
> have, but it doesn't seem like that's what most people want to do.
>
> I feel like we're finally in a position to get some things done here
> and this doesn't seem like the point to get stuck on. YMMV, of course.

I'd love to hear a response from Jacob and Heikki on my arguments
after their last response. But if after reading those arguments they
still think we should add a new function, I'll update the patchset to
include a new function.




Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-08-19 Thread Jelte Fennema-Nio
On Fri, 16 Aug 2024 at 16:43, Tom Lane  wrote:
> However, there are other ways to accomplish that.  I liked the
> suggestion of extending the CF webapp with a way to search for entries
> mentioning a particular mail message ID.  I dunno how hard it'd be to
> get it to recognize *any* message-ID from a thread, but surely at
> least the head message ought not be too hard to match.

I sent a patch to support this on the commitfest app to Magnus
off-list. It was pretty easy to implement, even for *any* message-ID.




Re: gitmaster server problem?

2024-08-19 Thread Jelte Fennema-Nio
On Mon, 19 Aug 2024 at 14:05, Jelte Fennema-Nio  wrote:
>
> On Mon, 19 Aug 2024 at 14:04, Robert Haas  wrote:
> > I'm still unable to access any https://git.postgresql.org/ URL.
>
> Yeah it's broken for me again too.

In case the actual error is helpful (although I doubt it):

Error 503 Backend fetch failed

Backend fetch failed

Guru Meditation:

XID: 1030914118



Varnish cache server




Re: gitmaster server problem?

2024-08-19 Thread Jelte Fennema-Nio
On Mon, 19 Aug 2024 at 14:04, Robert Haas  wrote:
> I'm still unable to access any https://git.postgresql.org/ URL.

Yeah it's broken for me again too.




Re: gitmaster server problem?

2024-08-19 Thread Jelte Fennema-Nio
On Mon, 19 Aug 2024 at 13:39, Joe Conway  wrote:
> What specifically? I am not seeing anything at the moment.

It seems to be working again fine. But before
https://git.postgresql.org was throwing 502 and 503 errors. Depending
on the request either by Varnish or nginx.




Re: gitmaster server problem?

2024-08-19 Thread Jelte Fennema-Nio
On Sat, 17 Aug 2024 at 22:05, Joe Conway  wrote:
> Just to close this out -- problem found and fixed about an hour ago.
> Sorry for the interruption.

Whatever the problem was it seems to have returned




Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

2024-08-19 Thread Jelte Fennema-Nio
On Mon, 19 Aug 2024 at 05:44, Robert Haas  wrote:
> I feel like what you're really complaining about here is that libpq is
> not properly versioned. We've just been calling it libpq.so.5 forever
> instead of bumping the version number when we change stuff.

There is PQlibVersion() that can be used for this. Which has existed
since 2010, so people can assume it exists.

> I just don't see why this particular change is special.

I didn't mean to say that it was, and I don't think the problem is
enormous either. I mainly meant to say that there is not just a cost
to Postgres maintainers when we introduce a new API. There's
definitely a cost to users and client authors too.

> Also, I kind of wish you had brought this argument up earlier. Maybe
> you did and I missed it, but I was under the impression that you were
> just arguing that "nobody will notice or care," which is a quite
> different argument than what you make here.

"nobody will notice or care" was definitely my argument before Jacob
responded. Since Jacob his response I realize there are two valid use
cases for PQprotocolVersion():

1. Feature detection. For this my argument still is: people won't
notice. Many people won't have bothered to use the function and
everyone else will have used >= 3 here.
2. Pinning the protocol version, because they care that the exact
protocol details are the same. Here people will have used == 3, and
thus their check will fail when we start to return a different version
from PQprotocolVersion(). But that's actually what this usecase
desires. By creating a new function, we actually break the expectation
of these people: i.e. they want the PQprotocolVersion() to return a
different version when the protocol changes.

Before Jacob responded I only considered the first case. So my
argument was indeed basically: Let's reuse this currently useless
function with the nice name, because no-one will care. But if people
thought that the risk was too high, I didn't see huge downsides to
introducing a new API either.

But **now I actually feel much more strongly about reusing the same
function**. Because by introducing a new function we actually break
the users of the second use-case.

P.S. The docs for PQprotocolVersion[1] have never said that this
function only returns the major protocol version. And by using the
word "Currently" it has always suggested that new return values could
be introduced later, and thus for feature detection you should use >=
3

[1]: 
https://www.postgresql.org/docs/13/libpq-status.html#LIBPQ-PQPROTOCOLVERSION




Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

2024-08-17 Thread Jelte Fennema-Nio
On Fri, 16 Aug 2024 at 19:44, Jacob Champion
 wrote:
> Keeping in mind that I'm responding to the change from 3 to 3:

Let me start with the fact that I agree we **shouldn't** change 3 to
3. And the proposed patch also specifically doesn't.

> https://github.com/search?q=PQprotocolVersion&type=code
> 
> https://github.com/psycopg/psycopg2/blob/658afe4cd90d3e167d7c98d22824a8d6ec895b1c/tests/test_async.py#L89

A **unittest** which is there just to add coverage for a method that
the driver exposes is not **actual usage** IMHO. Afaict Daniele (CCd
for confirmation) only added this code to add coverage for the API it
re-exposes. Considering this as an argument against returning
different values from this function is akin to saying that we should
avoid changing the function if we would have had coverage for this
function ourselves in our own libpq tests by checking for == 3.
Finally, this function in psycopg was only added in 2020. That's a
time when having this function return anything other than 3 when
connecting to a supported Postgres version was already not possible
for 10 years.

> 
> https://github.com/infusion/PHP/blob/7ebefb6426bb4b4820a30cca5c3a10bfd757b6ea/ext/pgsql/pgsql.c#L864
>
> where the old client is fine, but new clients could be tricked into
> writing similar checks as `>= 3` -- which is wrong because older
> libpqs use 3, haha, surprise, have fun with that!

This is indeed actual usage but, like any sensible production use, it
actually uses >= 3 so nothing would break even if we changed to using
3. Rewording what you're saying to make it sound much less
terrible: Users of the **new** API who fail to read the docs, and thus
use >= 3 instead of >=3, would then cause breakage when linking to
older libpq versions. This seems extremely unlikely for anyone to do.
Because if you are a new user of the API, then why on earth would you
check for 3.0 or larger. The last server that used a version lower
than 3.0 is 7.4 of which the final release was in 2010... So the only
reason to use PQprotocolVersion in new code would be to check for
versions higher than 3.0, for which the checks > 3, and >= 30001
would both work! And finally this would only be the case if we change
the definition of  3.0 to 3. Which as said above the proposed
patch specifically doesn't, to avoid such confusion.

> > The only example I could
> > find where someone used it at all was psycopg having a unittest for
> > their python wrapper around this API, and they indeed used == 3.
>
> I've written code that uses exact equality as well, because I cared
> about the wire protocol version.

So, if you cared about the exact wire protocol version for your own
usage. Then why wouldn't you want to know that the minor version
changed?

> Even if I hadn't, isn't the first
> public example enough, since a GitHub search is going to be an
> undercount? What is your acceptable level of breakage?

As explained above. Any actual usage that is not a **unittest** of a
driver library.

> People who are testing against this have some reason to care about the
> underlying protocol compatibility. I don't need to understand (or even
> agree with!) why they care; we've exported an API that allows them to
> do something they find useful.

Yes, and assuming they only care about major version upgrades seems
very presumptuous. If they manually parse packets themselves or want
package traces to output the same data, then they'd want to pin to an
exact protocol version, both minor and major. Just because we'd never
had a minor version bump before doesn't mean users of this API don't
care about being notified by them through the existing
PQprotocolVersion API.

> > The advantage is not introducing yet another API when we already have
> > one with a great name
>
> Sorry to move the goalposts, but when I say "value" I'm specifically
> talking about value for clients/community, not value for patch writers
> (or even maintainers). A change here doesn't add any value for
> existing clients when compared to a new API, since they've already
> written the version check code and are presumably happy with it. New
> clients that have reason to care about the minor version, whatever
> that happens to mean for a protocol, can use new code.

Not introducing new APIs definitely is useful to clients and the
community. Before users can use a new API, their libpq wrapper needs
to expose this new function that calls it through FFI. First of all
this requires work from client authors. But the **key point being**:
The new function would not be present in old libpq versions. So for
some clients, the FFI wrapper would also not exist for those old libpq
versions, or at least would fail. So now users before actually being
able to check for a minor protocol version, they first need an up to
date libpq wrapper library for their language that exposes the new
function, and then they'd still have to check their actual libpq
version, before they could final

Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

2024-08-16 Thread Jelte Fennema-Nio
On Fri, 16 Aug 2024 at 16:51, Heikki Linnakangas  wrote:
> That said, I think we *should* change the default for the time being, so
> that developers working on the bleeding edge and building from git get
> some exposure to it. Hopefully that will nudge some of the poolers to
> adopt NegotiateProtocolVersion sooner. But we revert the default to 3.0
> before the v18 release.

That seems reasonable to me. To be clear, the latest PgBouncer release
now supports NegotiateProtocolVersion.




Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

2024-08-16 Thread Jelte Fennema-Nio
On Fri, 16 Aug 2024 at 00:39, Jacob Champion
 wrote:
>
> On Thu, Aug 15, 2024 at 3:04 PM Heikki Linnakangas  wrote:
> > Perhaps we should even change it to return
> > 30 for protocol version 3.0, and just leave a note in the docs like
> > "in older versions of libpq, this returned 3 for protocol version 3.0".
>
> I think that would absolutely break current code. It's not uncommon
> (IME) for hand-built clients wrapping libpq to make sure they're not
> talking v2 before turning on some feature, and they're allowed to do
> that with a PQprotocolVersion() == 3 check. A GitHub code search
> brings up examples.

Can you give a link for that code search and/or an example where
someone used it like that in a real setting? The only example I could
find where someone used it at all was psycopg having a unittest for
their python wrapper around this API, and they indeed used == 3.

> As for 30001: I don't see the value in modifying an exported API in
> this way. Especially since we can add a new entry point that will be
> guaranteed not to break anyone, as Robert suggested. I think it's a
> POLA violation at minimum; my understanding was that up until this
> point, the value was incremented during major (incompatible) version
> bumps. And I think other users will have had the same understanding.

The advantage is not introducing yet another API when we already have
one with a great name that no-one is currently using. The current API
is in practice just a very convoluted way of writing 3. Also doing an
== 3 check is obviously problematic, if people use this function they
should be using > 3 to be compatible with future versions. So if we
ever introduce protocol version 4, then these (afaict theoretical)
users would break anyway.




Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-08-15 Thread Jelte Fennema-Nio
On Thu, 15 Aug 2024 at 15:33, Peter Eisentraut  wrote:
> Maybe this kind of thing should rather be on the linked-to web page, not
> in every email.

Yeah, I'll first put a code snippet on the page for the commitfest entry.

> But a more serious concern here is that the patches created by the cfbot
> are not canonical.  There are various heuristics when they get applied.
> I would prefer that people work with the actual patches sent by email,
> at least unless they know exactly what they are doing.  We don't want to
> create parallel worlds of patches that are like 90% similar but not
> really identical.

I'm not really sure what kind of heuristics and resulting differences
you're worried about here. The heuristics it uses are very simple and
are good enough for our CI. Basically they are:
1. Unzip/untar based on file extension
2. Apply patches using "patch" in alphabetic order

Also, when I apply patches myself, I use heuristics too. And my
heuristics are probably different from yours. So I'd expect that many
people using the exact same heuristic would only make the situation
better. Especially because if people don't know exactly what they are
doing, then their heuristics are probably not as good as the one of
our cfbot. I know I've struggled a lot the first few times when I was
manually applying patches.




Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-08-15 Thread Jelte Fennema-Nio
On Thu, 15 Aug 2024 at 15:28, Peter Eisentraut  wrote:
> How would you attach such an email to a thread?  Where in the thread
> would you attach it?  I'm not quite sure how well that would work.

My idea would be to have the commitfest app send it in reply to the
message id that was entered in the "New patch" page:
https://commitfest.postgresql.org/open/new/

> Maybe there could be a feature in the commitfest app to enter a message
> ID and get the commitfest entries that contain that message.

That's a good idea.




Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-08-15 Thread Jelte Fennema-Nio
On Thu, 15 Aug 2024 at 05:17, Euler Taveira  wrote:
> +1. Regarding the CI link, I would be good if the CF entry automatically adds 
> a
> link to the CI run. It can be a separate field or even add it to "Links".

I'm on it. I think this email should be a subset of the info on the CF
entry webpage, so I'll first change the cf entry page to include all
this info.

> I'm not sure about 4, you can always check the latest patch in the CF entry 
> (it
> is usually the "latest attachment") and that's what the cfbot uses to run.

This is definitely a personal preference thing, but I like reading
patches on GitHub much better than looking at raw patch files. It has
syntax highlighting and has those little arrow buttons at the top of a
diff, to show more context about the file.

I realized a 5th thing that I would want in the email and cf entry page

5. A copy-pastable set of git command that checks out the patch by
downloading it from the cfbot repo like this:

git config branch.cf/5107.remote
https://github.com/postgresql-cfbot/postgresql.git
git config branch.cf/5107.merge refs/heads/cf/5107
git checkout -b cf/5107
git pull

> If I understand your proposal correctly, there will be another email to the
> thread if the previous CF was closed and someone opened a new CF entry.
> Sometimes some CF entries are about the same thread.

Yeah, that's correct. If a new CF entry is created for an existing
thread a new email would be sent. But to be clear, if CF entry is
pushed to the next commitfest, **no** new email would be sent.




Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-08-15 Thread Jelte Fennema-Nio
On Thu, 15 Aug 2024 at 01:19, Tom Lane  wrote:
> +1 for the basic idea.

That's great to hear.

>  Not sure about your 3 & 4 points though, especially if that'd
> mean some delay in sending the mail.  For one thing, wouldn't those
> links be stale as soon as the initial patch got replaced?

I recently made those links predictable based on only the ID of the CF
entry. So they won't get stale, and I would want to send them straight
away (even if that means that they might return a 404 until cfbot
actually pushes the patches to github). The links would be:

https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf/$CFENTRYID

https://github.com/postgresql-cfbot/postgresql/compare/cf/$CFENTRYID~1...cf/$CFENTRYID

> Those links seem like they ought to live in the commitfest entry.

Agreed. I'll start with that, since that should be very easy.




Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-08-14 Thread Jelte Fennema-Nio
I'd like to send an automatic mail to a thread whenever it gets added
to a commitfest. Since this would impact everyone that's subscribed to
the mailinglist I'd like some feedback on this. This mail would
include:

1. A very short blurb like: "This thread was added to the commitfest
with ID 1234"
2. A link to the commitfest entry
3. A link to the cfbot CI runs
4. A link to the diff on GitHub
5. Any other suggestions?

The main reason I'm proposing this is that currently it's not trivial
to go from an email in my inbox to the commitfest entry. This can be
especially hard to do when the subject of the email is not the same as
the title of the commitfest entry. This then in turn increases the
barrier for people to change the commitfest status, to look at the CI
results, or look at the diff on GitHub. I also had once that I
accidentally added a thread twice to the commitfest, because I forgot
I had already added it a while back.

To be clear, this would **not** be a repeat email. It would be sent
only once per thread, i.e. it is sent when the thread is added to a
commitfest entry. So there won't be a flood of new emails you'll
receive.

Also the github link does not allow comments to be posted to github,
it's read-only. An example being:
https://github.com/postgresql-cfbot/postgresql/compare/cf/5025~1...cf/5025




Re: libpq: Fix lots of discrepancies in PQtrace

2024-08-14 Thread Jelte Fennema-Nio
On Wed, 14 Aug 2024 at 19:37, Alvaro Herrera  wrote:
> - to 0005 I change your pqTraceOutputEncryptionRequestResponse()
>   function name to pqTraceOutputCharResponse and instead of attaching
>   the "Response" literal in the outpuer to the name given in the
>   function call, just pass the whole string as argument to the function.

Fine by me

> - to 0006 I change function name pqFinishParsingMessage() to
>   pqParseDone() and reworded the commentary; also moved it to fe-misc.c.
>   Looks good otherwise.

The following removed comments seems useful to keep (I realize I
already removed them in a previous version of the patch, but I don't
think I did that on purpose)

-   /* Drop the processed message and loop around for another */

-   /* consume the message and exit */


-   /* Completed this message, keep going */
-   /* trust the specified message length as what to skip */


> - 0008 to fix NegotiateProtocolVersion looks correct per [1], but I
>   don't know how to test it.  Suggestions?

Two options:
1. Manually change code to make sure SendNegotiateProtocolVersion is
called in src/backend/tcop/backend_startup.c
2. Apply my patches from this thread[2] and use
max_protocol_version=latest in the connection string while connecting
to an older postgres server.

[2]: 
https://www.postgresql.org/message-id/flat/CAGECzQTyXDNtMXdq2L-Wp%3DOvOCPa07r6%2BU_MGb%3D%3Dh90MrfT%2BfQ%40mail.gmail.com#1b8cda3523555aafae89cc04293b8613




Re: Report search_path value back to the client.

2024-08-14 Thread Jelte Fennema-Nio
On Wed, 14 Aug 2024 at 18:22, Tomas Vondra  wrote:
> Here's the patch with a somewhat expanded / improved commit message.
> Jelte, can you take a look there's no silly mistake?

Looks good to me.




Create syscaches for pg_extension

2024-08-13 Thread Jelte Fennema-Nio
Shared libraries of extensions might want to invalidate or update their
own caches whenever a CREATE/ALTER/DROP EXTENSION command is run for
their extension (in any backend). Right now this is non-trivial to do
correctly and efficiently. But if the extension catalog was part of a
syscache this could simply be done by registering an callback using
CacheRegisterSyscacheCallback for the relevant syscache.

This change is only made to make the lives of extension authors easier.
The performance impact of this change should be negligible, since
updates to pg_extension are very rare.
From 06a21c34594351565a331d0a8353dcf78dc76159 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio 
Date: Tue, 13 Aug 2024 16:00:22 +0200
Subject: [PATCH v1] Create syscaches for pg_extension

Shared libraries of extensions might want to invalidate or update their
own caches whenever a CREATE/ALTER/DROP EXTENSION command is run for
their extension (in any backend). Right now this is non-trivial to do
correctly and efficiently. But if the extension catalog was part of a
syscache this could simply be done by registering an callback using
CacheRegisterSyscacheCallback for the relevant syscache.

This change is only made to make the lives of extension authors easier.
The performance impact of this change should be negligible, since
updates to pg_extension are very rare.
---
 src/include/catalog/pg_extension.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/include/catalog/pg_extension.h b/src/include/catalog/pg_extension.h
index cdfacc0930..673181b39a 100644
--- a/src/include/catalog/pg_extension.h
+++ b/src/include/catalog/pg_extension.h
@@ -56,4 +56,7 @@ DECLARE_TOAST(pg_extension, 4147, 4148);
 DECLARE_UNIQUE_INDEX_PKEY(pg_extension_oid_index, 3080, ExtensionOidIndexId, pg_extension, btree(oid oid_ops));
 DECLARE_UNIQUE_INDEX(pg_extension_name_index, 3081, ExtensionNameIndexId, pg_extension, btree(extname name_ops));
 
+MAKE_SYSCACHE(EXTENSIONOID, pg_extension_oid_index, 2);
+MAKE_SYSCACHE(EXTENSIONNAME, pg_extension_name_index, 2);
+
 #endif			/* PG_EXTENSION_H */
-- 
2.43.0



Re: format_datum debugging function

2024-08-12 Thread Jelte Fennema-Nio
On Mon, 12 Aug 2024 at 23:15, Paul Jungwirth
 wrote:
> On 8/12/24 04:32, Aleksander Alekseev wrote:
> >> (gdb) call format_datum(range_out, $1)
> >> $2 = 0x59162692d938 "[1,4)"
> >>
> >> I assume a patch like this doesn't need documentation. Does it need a 
> >> test? Anything else?
> >
> > I think you forgot to attach the patch. Or is it just a proposal?
>
> Sorry, patch attached here.

+1 for the idea. And the code looks trivial enough. I think this
should also contain a print_datum function too though.




Re: Returning from a rule with extended query protocol

2024-08-12 Thread Jelte Fennema-Nio
On Sun, 11 Aug 2024 at 23:54, Tom Lane  wrote:
> I think this might be the same issue recently discussed here:
>
> https://www.postgresql.org/message-id/flat/1df84daa-7d0d-e8cc-4762-85523e45e5e7%40mailbox.org

Yeah that's definitely the same issue.

> That discussion was leaning towards the idea that the cost-benefit
> of fixing this isn't attractive and we should just document the
> discrepancy.  However, with two reports now, maybe we should rethink.

I think it's interesting that both reports use rules in the same way,
i.e. to implement soft-deletes. That indeed seems like a pretty good
usecase for them. And since pretty much every serious client library
uses the extended query protocol, this breaks that usecase. But if
it's hard to fix, I'm indeed not sure if it's worth the effort. If we
don't we should definitely document it though, at CREATE RULE and in
the protocol spec.




Re: Returning from a rule with extended query protocol

2024-08-11 Thread Jelte Fennema-Nio
On Sun, 11 Aug 2024 at 13:29, Greg Rychlewski  wrote:
> I was testing creating a rule that uses RETURNING and noticed a difference 
> between the extended query protocol and the simple query protocol. In the 
> former, RETURNING is ignored (at least in my case) and the latter it is 
> respected:

That seems like a bug to me. The simple and extended protocol should
return the same data for the same query. I'm guessing CREATE RULE
isn't often enough for this difference to be noticed earlier. So yeah
please dig through the code and submit a patch to fix this.




Re: libpq: Fix lots of discrepancies in PQtrace

2024-08-10 Thread Jelte Fennema-Nio
On Sat, 10 Aug 2024 at 01:08, Alvaro Herrera  wrote:
> I don't want to add 4 bytes to struct pg_conn for tracing support.  I'm
> tempted to make the new struct member a plain 'char' to reduce overhead
> for a feature that almost nobody is going to use.  According to pahole
> we have a 3 bytes hole in that position of the struct, so if we make it
> a 1- or 2-byte member, there's no storage overhead whatsoever.

Sounds fine to me.

> Also, why not have pqTraceOutputMessage() responsible for resetting the
> byte after printing the message?  It seems to cause less undesirable
> detritus.

Yeah, that's indeed much nicer.

> I propose something like the attached, but it's as yet untested.  What
> do you think?

Looks good, but I haven't tested it yet either.




Re: Add trim_trailing_whitespace to editorconfig file

2024-08-09 Thread Jelte Fennema-Nio
On Fri, 9 Aug 2024 at 15:16, Peter Eisentraut  wrote:
> -*.sgml whitespace=space-before-tab,trailing-space,tab-in-indent
> -*.x[ms]l   whitespace=space-before-tab,trailing-space,tab-in-indent
> +*.py   
> whitespace=space-before-tab,trailing-space,tab-in-indent,tabwidth=4
> +*.sgml 
> whitespace=space-before-tab,trailing-space,tab-in-indent,tabwidth=1
> +*.xml  
> whitespace=space-before-tab,trailing-space,tab-in-indent,tabwidth=1
> +*.xsl  
> whitespace=space-before-tab,trailing-space,tab-in-indent,tabwidth=2
>
> Why add tabwidth settings to files that are not supposed to contain tabs?

That's there so that the generated .editorconfig file the correct
indent_size. I guess another approach would be to change the
generate_editorconfig.py script to include hardcoded values for these
4 filetypes.




Re: Partial aggregates pushdown

2024-08-08 Thread Jelte Fennema-Nio
SUMMARY OF THREAD

The design of patch 0001 is agreed upon by everyone on the thread (so
far). This adds the PARTIAL_AGGREGATE label for aggregates, which will
cause the finalfunc not to run. It also starts using PARTIAL_AGGREGATE
for pushdown of aggregates in postgres_fdw. In 0001 PARTIAL_AGGREGATE
is only supported for aggregates with a non-internal/pseudo type as
the stype.

The design for patch 0002 is still under debate. This would expand on
the functionality added by adding support for PARTIAL_AGGREGATE for
aggregates with an internal stype. This is done by returning a byte
array containing the bytes that the serialfunc of the aggregate
returns.

A competing proposal for 0002 is to instead change aggregates to not
use an internal stype anymore, and create dedicated types. The main
downside here is that infunc and outfunc would need to be added for
text serialization, in addition to the binary serialization. An open
question is: Can we change the requirements for CREATE TYPE, so that
types can be created without infunc and outfunc.

WHAT IS NEEDED?

The things needed for this patch are that docs need to be added, and
detailed codereview needs to be done.

Feedback from more people on the two competing proposals for 0002
would be very helpful in making a decision.




Re: Add trim_trailing_whitespace to editorconfig file

2024-08-07 Thread Jelte Fennema-Nio
On Wed, 7 Aug 2024 at 21:09, Andrew Dunstan  wrote:
> You're not meant to use our pg_bsd_indent on its own without the
> appropriate flags, namely (from src/tools/pgindent/pgindent):

Ah sorry, I wasn't clear in what I meant then. I meant that if you
look at the sources of pg_bsd_indent (such as
src/tools/pg_bsd_indent/io.c) then you'll realize that comments are
alligned using tabs of width 8, not tabs of width 4. And right now
.editorconfig configures editors to show all .c files with a tab_width
of 4, because we use that for Postgres source files. The bottom
.gitattributes line now results in an editorconfig rule that sets a
tab_width of 8 for just the c and h files in src/tools/pg_bsd_indent
directory.

> Also, why are you proposing to undet indent-style for .pl and .pm files?
> That's not in accordance with our perltidy settings
> (src/tools/pgindent/perltidyrc), unless I'm misunderstanding.

All the way at the bottom of the .editorconfig file those "ident_style
= unset" lines are overridden to be "tab" for .pl and .pm files.
There's a comment there explaining why it's done that way.

# We want editors to use tabs for indenting Perl files, but we cannot add it
# such a rule to .gitattributes, because certain lines are still indented with
# spaces (e.g. SYNOPSIS blocks).
[*.{pl,pm}]
indent_style = tab

But now thinking about this again after your comment, I realize it's
just as easy and effective to change the script slightly to hardcode
the indent_style for "*.pl" and "*.pm" so that the resulting
.editorconfig looks less confusing. Attached is a patch that does
that.

I also added a .gitattributes rule for .py files, and changed the
default tab_width to unset. Because I realized the resulting
.editorconfig was using tab_width 8 for python files when editing
src/tools/generate_editorconfig.py


v7-0001-Add-script-to-keep-.editorconfig-in-sync-with-.gi.patch
Description: Binary data


Re: Add trim_trailing_whitespace to editorconfig file

2024-08-07 Thread Jelte Fennema-Nio
On Tue, 9 Apr 2024 at 12:42, Jelte Fennema-Nio  wrote:
> Okay, I spent the time to add a script to generate the editorconfig
> based on .gitattributes after all. So attached is a patch that adds
> that.

I would love to see this patch merged (or at least some feedback on
the latest version). I think it's pretty trivial and really low risk
of breaking anyone's workflow, and it would *significantly* improve my
own workflow.

Matthias mentioned on Discord that our vendored in pg_bsd_indent uses
a tabwidth of 8 and that was showing up ugly in his editor. I updated
the patch to include a fix for that too.


v6-0001-Add-script-to-keep-.editorconfig-in-sync-with-.gi.patch
Description: Binary data


Re: Official devcontainer config

2024-08-01 Thread Jelte Fennema-Nio
On Thu, 1 Aug 2024 at 16:56, Junwang Zhao  wrote:
> I post my daily used devcontainer config[2] , Jelte(ccd)
> suggested that it might be a good idea we integrate the
> config into postgres repo so that the barrier to entry for
> new developers will be much lower.

In my experience adding a devcontainer config has definitely made it
easier for people to contribute to Citus. So thank you for working on
this! This is not a full review, but an initial pass.

> After diving into the container, the postCreateCommand.sh
> will be automatically called, it will do some configurations
> like git, perf, .vscode, core_pattern, etc. It also downloads
> michaelpq's pg_plugins and FlameGraph.

I think the .git settings don't fit well here, they are mostly aliases
which are very much based on personal preference and not related to
Postgres development. It seems better recommend users to use the
devcontainer dotfiles support for this:
https://code.visualstudio.com/docs/devcontainers/containers#_personalizing-with-dotfile-repositories

> - pgindent

It might make sense to install Tristan (ccd) his Postgres Hacker
extension for vscode to make this a bit more userfriendly:
https://marketplace.visualstudio.com/items?itemName=tristan957.postgres-hacker




Re: Support prepared statement invalidation when result types change

2024-07-25 Thread Jelte Fennema-Nio
On Wed, 24 Jul 2024 at 17:39, Tom Lane  wrote:
> > This patch starts to allow a prepared statement to continue to work even
> > when the result type changes.
>
> What this is is a wire protocol break.

Yeah that's what I realised as well in my latest email. I withdrew
this patch from the commitfest now to reflect that. Until we get the
logic for protocol bumps in:
https://www.postgresql.org/message-id/flat/CAGECzQQPQO9K1YeBKe%2BE8yfpeG15cZGd3bAHexJ%2B6dpLP-6jWw%40mail.gmail.com#2386179bc970ebaf1786501f687a7bb2

> What if the client has
> previously done a Describe Statement on the prepared statement?
> We have no mechanism for notifying it that that information is
> now falsified.  The error is thrown to prevent us from getting
> into a situation where we'd need to do that.

However, this makes me think of an intermediary solution. In some
sense it's only really a protocol break if the result type changes
between the last Describe and the current Execute. So would it be okay
if a Describe triggers the proposed invalidation?




Re: [PATCH] GROUP BY ALL

2024-07-24 Thread Jelte Fennema-Nio
On Tue, 23 Jul 2024 at 15:22, Andrei Borodin  wrote:
> I'd like to have GROUP BY AUTO (I also proposed version GROUP BY SURPRISE 
> ME). But I wouldn't like to open pandora box of syntax sugar extensions which 
> may will be incompatible with future standards.
> If we could have extensible grammar - I'd be happy to have a lot of such 
> enhancements. My top 2 are FROM table SELECT column and better GROUP BY.

Personally my number one enhancement would be allowing a trailing
comma after the last column in the SELECT clause.




Re: [PATCH] GROUP BY ALL

2024-07-24 Thread Jelte Fennema-Nio
On Mon, 22 Jul 2024 at 22:55, David Christensen  wrote:
> I see that there'd been some chatter but not a lot of discussion about
> a GROUP BY ALL feature/functionality.  There certainly is utility in
> such a construct IMHO.

+1 from me. When exploring data, this is extremely useful because you
don't have to update the GROUP BY clause every time

Regarding the arguments against this:
1. I don't think this is any more unreadable than being able to GROUP
BY 1, 2, 3. Or being able to use column aliases from the SELECT in the
GROUP BY clause. Again this is already allowed. Personally I actually
think it's more readable than specifying e.g. 5 columns in the group
by, because then I have to cross-reference with columns in the SELECT
clause to find out if they are the same. With ALL I instantly know
it's grouped by all
2. This is indeed not part of the standard. But we have many things
that are not part of the standard. I think as long as we use the same
syntax as snowflake, databricks and duckdb I personally don't see a
big problem. Then we could try and make this be part of the standard
in the next version of the standard.




Re: Report search_path value back to the client.

2024-07-20 Thread Jelte Fennema-Nio
On Fri, 19 Jul 2024 at 21:48, Tomas Vondra
 wrote:
>
>
>
> On 7/19/24 17:16, Jelte Fennema-Nio wrote:
> > That's been moving forward, even relatively fast imho for the
> > size/impact of that patchset. But those changes are much less
> > straight-forward than this patch. And while I hope that they can get
> > committed for PG18 this year, I'm not confident about that.
>
> I don't know. My experience is that whenever we decided to not do a
> simple improvement because a more complex patch was expected to do
> something better/smarter, we often ended up getting nothing.
>
> So maybe it'd be best to get this committed, and then if the other patch
> manages to get into PG18, we can revert this change (or rather that
> patch would do that).

Yeah, I think we're aligned on this. Because that's totally what I
meant to say, but I don't seem to have written down that conclusion
explicitly.

> Maybe it's crazy-talk, but couldn't we make the GUC settable exactly
> once? That should be doable using the GUC hooks, I think. That means
> pgbouncer would set the GUC right after opening the connection, and then
> the following attempts to set it would either error out, or silently do
> nothing?
>
> Perhaps it could even allow enabling reporting for more GUCs, but once a
> GUC is on the list, it's reported forever.

This could maybe work, I'll think a bit about it. The main downside I
see is that PgBouncer can then not act as a transparent proxy, because
it cannot reset value to the value that the client expects the GUC to
be. But in practice that doesn't matter much for this specific case
because all that happens in this case is that the client gets a few
more ParameterStatus messages that it did not want.




Re: Report search_path value back to the client.

2024-07-19 Thread Jelte Fennema-Nio
On Thu, 18 Jul 2024 at 22:47, Tomas Vondra
 wrote:
> That being said, it this makes using pgbouncer easier (or even possible
> in some applications where it currently does not work), I'd vote to get
> this committed.

It definitely is a pain point for some setups. A speaker brought it up
at FOSDEM pgday too, the speaker was talking about multi-tenant/SaaS
applications.

> So, did that other patch move forward, in some way? The last message is
> from January, I'm not sure I want to read through that thread just to
> find out it's stuck on something.

Basically, the discussion continued on this commitfest entry:
https://commitfest.postgresql.org/48/4736/

That's been moving forward, even relatively fast imho for the
size/impact of that patchset. But those changes are much less
straight-forward than this patch. And while I hope that they can get
committed for PG18 this year, I'm not confident about that.

> Also, I recall we had a session at pgconf.dev to discuss this protocol
> stuff. I don't remember what the conclusions from that part were :-(
>
> Stupid idea - could we have a GUC/function/something to define which
> GUCs the client wants to get reports for? Maybe that'd be simpler and
> would not require protocol changes? Not as pretty, of course, and maybe
> it has some fatal flaw.

The GUC idea was proposed before, but that has the flaw that a user
with SQL access can change this GUC without the client-library or
pooler realizing. Maybe that's okay, especially if it's only additive
to the current defaults (e.g. removing client_encoding from the list
is bound to cause issues for many clients). Basically the protocol
change is to add support for protocol parameters, which can only be
set at the protocol level and not the SQL level. One of such protocol
parameters would then have the same behaviour as the GUC that you're
describing (except that you cannot change the value using SQL).

> In any case, I think it'd be good to decide what to do with this patch.
> Whether to reject it or get it committed, even if we hope to get a
> better / extensible solution in the future. I'd vote to commit.

Sounds good to me.

> What concerns me a little bit is if this will make our life more
> complicated in the future. I mean, imagine we get it committed, and then
> get the protocol stuff later. Wouldn't that mean pgbouncer needs to do
> three different things, depending on the server version? (without
> search_path reporting, with reporting and with the new protocol stuff?)

For PgBouncer (or other clients) its perspective I don't see any
issues here. The other protocol stuff would still reuse the same
messages, and so PgBouncer can track the search_path if it receives
the update for the search_path (and not track it if it doesn't). So
imho there are basically no downsides to committing this patch. The
only impact it has is that if people change their search_path in a
query, then they'll get slightly more network traffic back than before
this patch.




Re: Set log_lock_waits=on by default

2024-07-19 Thread Jelte Fennema-Nio
Late to the party, but +1 from me on this default change also

On Fri, 19 Jul 2024 at 11:54, Laurenz Albe  wrote:

> On Thu, 2024-07-18 at 12:25 +0200, Michael Banck wrote:
> > this patch is still on the table, though for v18 now.
> >
> > Nathan mentioned up-thread that he was hesitant to commit this without
> > further discussion. Laurenz, Stephen and I are +1 on this, but when it
> > comes to committers having chimed in only Tom did so far and was -1.
> >
> > Are there any others who have an opinion on this?
>
> If we want to tally up, there were also +1 votes from Christoph Berg,
> Shinya Kato, Nikolay Samokhvalov, Jeremy Schneider and Frédéric Yhuel.
>
> The major criticism is Tom's that it might unduly increase the log volume.
>
> I'm not trying to rush things, but I see little point in kicking a trivial
> patch like this through many commitfests.  If no committer wants to step
> up and commit this, it should be rejected.
>
> Yours,
> Laurenz Albe
>
>
>


Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal

2024-07-10 Thread Jelte Fennema-Nio
On Wed, 10 Jul 2024 at 16:46, Nathan Bossart  wrote:
> Do we actually need to look at pmState?  Or could we just skip
> it if the context is <= PGC_S_ARGV?

I'm not 100% sure, but I think PGC_S_FILE would still be used when
postgresql.conf changes and on SIGHUP is sent. And we would want the
check_hook to be used then.




Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal

2024-07-10 Thread Jelte Fennema-Nio
On Wed, 10 Jul 2024 at 16:18, Nathan Bossart  wrote:
> Yeah.  I initially thought this patch might be okay, at least as a stopgap,
> but Jelte pointed out a case where it doesn't work, namely when you have
> something like the following in the config file:
>
> wal_level = 'minimal'
> summarize_wal = 'true'
> wal_level = 'logical'

I think that issue can be solved fairly easily by making the guc
check_hook always pass during postmaster startup (by e.g. checking
pmState), and relying on the previous startup check instead during
startup.




Re: [EXTERNAL] Re: Add non-blocking version of PQcancel

2024-07-10 Thread Jelte Fennema-Nio
On Mon, 1 Jul 2024 at 00:38, Jelte Fennema-Nio  wrote:
> Ugh yes, I think this was a copy paste error. See attached patch 0003
> to fix this (rest of the patches are untouched from previous
> revision).

Alvaro committed 0003, which caused cfbot to think a rebase is
necessary. Attached should solve that.


v4-0001-Make-postgres_fdw-cancel-test-not-flaky-anymore.patch
Description: Binary data


v4-0002-Do-not-reset-statement_timeout-indicator-outside-.patch
Description: Binary data


Re: Partial aggregates pushdown

2024-07-08 Thread Jelte Fennema-Nio
On Mon, 8 Jul 2024 at 14:12, fujii.y...@df.mitsubishielectric.co.jp
 wrote:
> The best thing about Approach2 is that it lets us send state values using the 
> existing data type system.
> I'm worried that if we choose Approach2, we might face some limits because we 
> have to create new types.
> But, we might be able to fix these limits if we look into it more.
>
> Approach1 doesn't make new types, so we can avoid these limits.
> But, it means we have to make export/import functions that are similar to the 
> typsend/typreceive functions.
> So, we need to make sure if we really need this method.
>
> Is this the right understanding?

Yeah, correct. To clarify my reasoning a bit more: IMHO, the main
downside of implementing Approach1 is that we then end up with two
different mechanisms to "take data from memory and serialize it in a
way in which it can be sent over the network". I'd very much prefer if
we could have a single system responsible for that task. So if there's
issues with the current system (e.g. having to implement
typinput/typoutput), then I'd rather address these problems in the
existing system. Only if that turns out to be impossible for some
reason, then I think I'd prefer Approach1.

Personally, even if the Approach2 requires a bit more code, then I'd
still prefer a single serialization system over having two
serializiation systems.




Re: Partial aggregates pushdown

2024-07-08 Thread Jelte Fennema-Nio
On Sun, 7 Jul 2024 at 23:52, fujii.y...@df.mitsubishielectric.co.jp
 wrote:
> My plan for advancing the patch involves the following steps:
>   Step1. Decide the approach on transmitting state value.
>   Step2. Implement code (including comments) and tests to support a subset of 
> aggregate functions.
> Specifically, I plan to support avg, sum, and other aggregate functions 
> like min and max which don't need export/import functions.
>   Step3. Add documentations.
>
> To clarify my approach, should I proceed with Step 3 before Step2?

(my opinion, Bruce might have a different one)

I think it's good that you split the original patch in two:
0001: non-internal partial aggregates
0002: internal partial aggregates

I think we're aligned on the general design of 0001. So I think now is
definitely the time to include documentation there, so we can discuss
this patch in more detail, and move it forward.

I think generally for 0002 it would also be useful to have
documentation, I personally like reading it to understand the general
design and then comparing that to the code. But I also understand that
the language differences between Japanese and English, makes writing
such docs a significant effort for you. So I think it would be fine to
skip docs for 0002 for now until we decide on the approach we want to
take for internal partial aggregates.




Re: Partial aggregates pushdown

2024-07-08 Thread Jelte Fennema-Nio
On Sun, 30 Jun 2024 at 23:42, fujii.y...@df.mitsubishielectric.co.jp
 wrote:
> Instead, I can make PARTIAL_AGGREGATE an unreserved word by placing it after 
> the FILTER clause, like avg(c1) FILTER (WHERE c2 > 0) PARTIAL_AGGREGATE, and 
> by marking it as an ASLABEL word like FILTER.
> I attached the patch of the method.
> If there are no objections, I would like to proceed with the method described 
> above.
> I'd appreciate it if anyone comment the method.

I like this approach of using PARTIAL_AGGREGATE in the same place as
the FILTER clause.




Re: Partial aggregates pushdown

2024-07-08 Thread Jelte Fennema-Nio
On Sun, 7 Jul 2024 at 23:46, fujii.y...@df.mitsubishielectric.co.jp
 wrote:
> In my mind, Approach1 is superior. Therefore, if there are no objections this 
> week, I plan to resume implementing Approach1 next week. I would appreciate 
> it if anyone could discuss the topic with me or ask questions.

Honestly, the more I think about this, the more I like Approach2. Not
because I disagree with you about some of the limitations of
Approach2, but because I'd rather see those limitations fixed in
CREATE TYPE, instead of working around these limitations in CREATE
AGGREGATE. That way more usages can benefit. Detailed explanation and
arguments below.

> 1. Extendability
> I believe it is crucial to support scenarios where the local and remote major 
> versions may differ in the future (see the below).
>
> https://www.postgresql.org/message-id/4012625.1701120204%40sss.pgh.pa.us

>From my reading, Tom's concern is that different server versions
cannot talk to each other anymore. So as long as this perf
optimization is only enabled when server versions are the same, I
don't think there is a huge problem if we never implement this.
Honestly, I even think making this optimization opt-in at the FDW
server creation level would already solve Tom's concert. I do agree
that it would be good if we could have cross version partial
aggregates though, so it's definitely something to consider.

> Regarding this aspect, I consider Approach1 superior to Approach2. The reason 
> is that:
> ・The data type of an aggregate function's state value may change with each 
> major version increment.
> ・In Approach1, by extending the export/import functionalities to include the 
> major version in which the state value was created (refer to p.16 and p.17 of 
> [1]), I can handle such situations.
> ・On the other hand, it appears that Approach2 fundamentally lacks the 
> capability to support these scenarios.

Approach 2 definitely has some cross-version capabilities, e.g.
jsonb_send includes a version. Such an approach can be used to solve a
newer coordinator talking to an older worker, if the transtypes are
the same.

I personally don't think it's worth supporting this optimization for
an older coordinator talking to a newer worker. Using binary protocol
to talk to from an older server to a newer server doesn't work either.

Finally, based on p.16 & p.17 it's unclear to me how cross-version
with different transtypes would work. That situation seems inherently
incompatible to me.

> 2. Amount of codes
> Regarding this aspect, I find Approach1 to be better than Approach2.
> In Approach1, developers only need to export/import functions and can use a 
> standardized format for transmitting state values.
> In Approach2, developers have two options:
>   Option1: Adding typinput/typoutput and typsend/typreceive.
>   Option2: Adding typinput/typoutput only.
> Option1 requires more lines of code, which may be seen as cumbersome by some 
> developers.
> Option2 restricts developers to using only text representation for 
> transmitting state values, which I consider limiting.

In my opinion this is your strongest argument for Approach1. But you
didn't answer my previous two questions yet:

On Tue, 25 Jun 2024 at 11:28, Jelte Fennema-Nio  wrote:
> So that leaves me two questions:
> 1. Maybe CREATE TYPE should allow types without input/output functions
> as long as send/receive are defined. For these types text
> representation could fall back to the hex representation of bytea.
> 2. If for some reason 1 is undesired, then why are transtypes so
> special. Why is it fine for them to only have send/receive functions
> and not for other types?

Basically: I agree with this argument, but I feel like this being a
problem for this usecase is probably a sign that we should take the
solution a step further and solve this at the CREATE TYPE level
instead of allowing people to hack around CREATE TYPE its limitations
just for these partial aggregates.

> 3. Catalog size
> Regarding this point, I believe Approach1 is better than Approach2.
> In Approach1, theoretically, it is necessary to add export/import functions 
> to pg_proc for each aggregate.
> In Approach2, theoretically, it is necessary to add typoutput/typinput 
> functions (and typsend/typreceive if necessary) to pg_proc and add a native 
> type to pg_type for each aggregate.
> I would like to emphasize that we should consider user-defined functions in 
> addition to built-in aggregate functions.
> I think most developers prefer to avoid bloating catalogs, even if they may 
> not be able to specify exact reasons.
> In fact, in Robert's previous review, he expressed a similar concern (see 
> below).
>
> https://www.postgresql.org/message-id/CA%2BTgmobvja%2Bjytj5zcEcYgqzOaeJiqrrJxgqDf1q%3D

Re: Make query cancellation keys longer

2024-07-04 Thread Jelte Fennema-Nio
On Thu, 4 Jul 2024 at 14:43, Tomas Vondra  wrote:
> I don't have any immediate feedback regarding this patch, but I'm
> wondering about one thing related to cancellations - we talk cancelling
> a query, but we really target a PID (or a particular backend, no matter
> how we identify it).
>
> I occasionally want to only cancel a particular query, but I don't think
> that's really possible - the query may complete meanwhile, and the
> backend may even get used for a different user connection (e.g. with a
> connection pool)? Or am I missing something important?

No, you're not missing anything. Having the target of the cancel
request be the backend instead of a specific query is really annoying
and can cause all kinds of race conditions. I had to redesign and
complicate the cancellation logic in PgBouncer significantly, to make
sure that one client could not cancel a connection from another client
anymore: https://github.com/pgbouncer/pgbouncer/pull/717

> Anyway, I wonder if making the cancellation key longer (or variable
> length) might be useful for this too - it would allow including some
> sort of optional "query ID" in the request, not just the PID. (Maybe
> st_xact_start_timestamp would work?)

Yeah, some query ID would be necessary. I think we'd want a dedicated
field for it though. Instead of encoding it in the secret. Getting the
xact_start_timestamp would require communication with the server to
get it, which you would like to avoid since the server might be
unresponsive. So I think a command counter that both sides keep track
of would be better. This counter could then be incremented after every
Query and Sync message.

> Obviously, that's not up to this patch, but it's somewhat related.

Yeah, let's postpone more discussion on this until we have the
currently proposed much simpler change in, which only changes the
secret length.




Re: Make query cancellation keys longer

2024-07-04 Thread Jelte Fennema-Nio
On Thu, 4 Jul 2024 at 12:32, Heikki Linnakangas  wrote:
> We currently don't do any locking on the ProcSignal array. For query
> cancellations, that's good because a query cancel packet is processed
> without having a PGPROC entry, so we cannot take LWLocks. We could use
> spinlocks though. In this patch, I used memory barriers to ensure that
> we load/store the pid and the cancellation key in a sensible order, so
> that you cannot e.g. send a cancellation signal to a backend that's just
> starting up and hasn't advertised its cancellation key in ProcSignal
> yet. But I think this might be simpler and less error-prone by just
> adding a spinlock to each ProcSignal slot. That would also fix the
> existing race condition where we might set the pss_signalFlags flag for
> a slot, when the process concurrently terminates and the slot is reused
> for a different process. Because of that, we currently have this caveat:
> "... all the signals are such that no harm is done if they're mistakenly
> fired". With a spinlock, we could eliminate that race.

I think a spinlock would make this thing a whole concurrency stuff a
lot easier to reason about.

+   slot->pss_cancel_key_valid = false;
+   slot->pss_cancel_key = 0;

If no spinlock is added, I think these accesses should still be made
atomic writes. Otherwise data-races on those fields are still
possible, resulting in undefined behaviour. The memory barriers you
added don't prevent that afaict. With atomic operations there are
still race conditions, but no data-races.

Actually it seems like that same argument applies to the already
existing reading/writing of pss_pid: it's written/read using
non-atomic operations so data-races can occur and thus undefined
behaviour too.

-volatile pid_t pss_pid;
+pid_tpss_pid;

Why remove the volatile modifier?




Re: Make query cancellation keys longer

2024-07-04 Thread Jelte Fennema-Nio
On Thu, 4 Jul 2024 at 12:35, Heikki Linnakangas  wrote:
>
> On 04/07/2024 13:32, Heikki Linnakangas wrote:
> > Here's a new version of the first patch.
>
> Sorry, forgot attachment.

It seems you undid the following earlier change. Was that on purpose?
If not, did you undo any other earlier changes by accident?

> > +SendCancelRequest(int backendPID, int32 cancelAuthCode)
> >
> > I think this name of the function is quite confusing, it's not sending
> > a cancel request, it is processing one. It sends a SIGINT.
>
> Heh, well, it's sending the cancel request signal to the right backend,
> but I see your point. Renamed to ProcessCancelRequest.




Re: Fix a comment on PQcancelErrorMessage

2024-07-04 Thread Jelte Fennema-Nio
On Thu, 4 Jul 2024 at 06:46, Yugo NAGATA  wrote:
> Attached is a small patch to fix a comment on PQcancelErrorMessage.

Oops, copy paste mistake on my part I guess. New comment LGTM




Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal

2024-07-03 Thread Jelte Fennema-Nio
On Wed, 3 Jul 2024 at 16:46, Jelte Fennema-Nio  wrote:
>
> Ugh copy paste mistake, this is what I meant
>
> On Wed, 3 Jul 2024 at 16:45, Jelte Fennema-Nio  wrote:
> > This hits the already existing check:
> > summarize_wal = 'true'
> > wal_level = 'minimal'
> >
> > This hits the new check:
> > wal_level = 'minimal'
> > summarize_wal = 'true'
> >
> > And actually this would throw an error from the new check even though
> > the config is fine:
> >
> > wal_level = 'minimal'
> > summarize_wal = 'true'
> > wal_level = 'logical'

Okay... fixed one more copy paste mistake... (I blame end-of-day brain)




Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal

2024-07-03 Thread Jelte Fennema-Nio
Ugh copy paste mistake, this is what I meant

On Wed, 3 Jul 2024 at 16:45, Jelte Fennema-Nio  wrote:
> This hits the already existing check:
> summarize_wal = 'true'
> wal_level = 'minimal'
>
> This hits the new check:
> summarize_wal = 'true'
> wal_level = 'minimal'
>
> And actually this would throw an error from the new check even though
> the config is fine:
>
> wal_level = 'minimal'
> summarize_wal = 'true'
> wal_level = 'logical'




Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal

2024-07-03 Thread Jelte Fennema-Nio
On Wed, 3 Jul 2024 at 16:30, Nathan Bossart  wrote:
> IME these sorts of GUC hooks that depend on the value of other GUCs tend to
> be quite fragile.  This one might be okay because wal_level defaults to
> 'replica' and because there is an additional check in postmaster.c, but
> that at least deserves a comment.

Yeah, this hook only works because wal_level isn't PGC_SIGHUP and
indeed because there's a check in postmaster.c. It now depends on the
ordering of these values in your config which place causes the error
message on startup.

This hits the already existing check:
summarize_wal = 'true'
wal_sumarizer = 'minimal'

This hits the new check:
summarize_wal = 'true'
wal_sumarizer = 'minimal'

And actually this would throw an error from the new check even though
the config is fine:

wal_sumarizer = 'minimal'
summarize_wal = 'true'
wal_sumarizer = 'logical'

> This sort of thing comes up enough that perhaps we should add a
> better-supported way to deal with GUCs that depend on each other...

+1. Sounds like we need a global GUC consistency check




Re: [PATCH] Handle SK_SEARCHNULL and SK_SEARCHNOTNULL in HeapKeyTest

2024-07-02 Thread Jelte Fennema-Nio
On Tue, 2 Jul 2024 at 10:15, Aleksander Alekseev
 wrote:
> The referred patch was rejected at first because it didn't modify
> nodeSeqScan.c to make use of the change within the core.

I guess we interpret Heikis email differently. I read it as: "If this
improves performance, then let's also start using it in core. If not,
why do extensions need it?" And I think you quite clearly explained
that even if perf is not better, then the usability for extensions
that don't want to use SPI is better.

I don't think Heiki meant his response as not using it in core being a
blocker for the patch. But maybe my interpretation of his response is
incorrect.




Re: [PATCH] Handle SK_SEARCHNULL and SK_SEARCHNOTNULL in HeapKeyTest

2024-07-02 Thread Jelte Fennema-Nio
On Mon, 1 Jul 2024 at 15:48, Aleksander Alekseev
 wrote:
> As I recall, previously it was argued that changes like this should
> have some use within the core [1].

I don't see that argument anywhere in the thread honestly. I did see
heiki asking why it would be useful for extensions, but that was
answered there.




Re: doc: modify the comment in function libpqrcv_check_conninfo()

2024-07-01 Thread Jelte Fennema-Nio
On Thu, 27 Jun 2024 at 12:27, ikedarintarof
 wrote:
> Thanks for your suggestion. I used ChatGPT to choose the wording, but
> it's still difficult for me.

Looks good to me now (but obviously biased since you took my wording).
Adding Robert, since he authored the commit that introduced this
comment.




Re: [EXTERNAL] Re: Add non-blocking version of PQcancel

2024-06-30 Thread Jelte Fennema-Nio
On Sun, 30 Jun 2024 at 21:00, Noah Misch  wrote:
> Shouldn't that be s/conn->status/cancelConn->status/?

Ugh yes, I think this was a copy paste error. See attached patch 0003
to fix this (rest of the patches are untouched from previous
revision).


v3-0001-Make-postgres_fdw-cancel-test-not-flaky-anymore.patch
Description: Binary data


v3-0003-Fix-copy-paste-mistake-in-PQcancelCreate.patch
Description: Binary data


v3-0002-Do-not-reset-statement_timeout-indicator-outside-.patch
Description: Binary data


Re: Converting README documentation to Markdown

2024-06-29 Thread Jelte Fennema-Nio
On Fri, 28 Jun 2024 at 20:40, Peter Eisentraut  wrote:
> I have my "less" set up so that "less somefile.md" automatically renders
> the markdown.  That's been pretty useful.  But if that now keeps making
> a mess out of PostgreSQL's README files, then I'm going to have to keep
> fixing things, and I might get really mad.  That's the worst that could
> happen. ;-)

Do you have reason to think that this is going to be a bigger issue
for Postgres READMEs than for any other markdown files you encounter?
Because this sounds like a generic problem you'd run into with your
"less" set up, which so far apparently has been small enough that it's
worth the benefit of automatically rendering markdown files.

> So I don't agree with "aspirational markdown".  If we're going to do it,
> then I expect that the files are marked up correctly at all times.

I think for at least ~90% of our README files this shouldn't be a
problem. If you have specific ones in mind that contain difficult
markup/diagrams, then maybe we shouldn't convert those.

> Conversely, what's the best that could happen?

That your "less" would automatically render Postgres READMEs nicely.
Which you say has been pretty useful ;-) And maybe even show syntax
highlighting for codeblocks.

P.S. Now I'm wondering what your "less" is.




Re: cfbot update: Using GitHub for patch review

2024-06-29 Thread Jelte Fennema-Nio
On Sat, 29 Jun 2024 at 01:13, Thomas Munro  wrote:
>
> On Sat, Jun 29, 2024 at 1:10 AM Ashutosh Bapat
>  wrote:
> > I need to sign in to github to add my review comments. So those who do not 
> > have a github account can not use it for review. But I don't think that can 
> > be fixed. We need a way to know who left review comments.
>
> I don't think Jelte was talking about moving review discussion to
> Github, just providing a link to *view* the patches there.

Totally correct. And I realize now I should have called that out
explicitly in the initial email.

While I personally would love to be able to read & write comments on a
Github PR, integrating that with the mailing list in a way that the
community is happy with as a whole is no small task (both technically
and politically).

So (for now) I took the easy way out and sidestepped all those
difficulties, by making the github branches of the cfbot (which we
already had) a bit more user friendly as a way to access patches in a
read-only way.

>  Now I'm
> wondering if there is a way to disable comments on commits in the
> postgresql-cfbot GH account.  I guess they'd be lost after 48 hours
> anyway when the branch gets force-pushed and commit hash changes?  I
> don't want people to start posting comments there that no one is
> looking at.

It seems you can disable them for 6 months at a time here:
https://github.com/postgresql-cfbot/postgresql/settings/interaction_limits




Re: Converting README documentation to Markdown

2024-06-28 Thread Jelte Fennema-Nio
On Fri, 28 Jun 2024 at 09:38, Peter Eisentraut  wrote:
> Getting that right in Markdown can be quite tricky.

I agree that in some cases it's tricky. But what's the worst case that
can happen when you get it wrong? It renders weird on github.com.
Luckily there's a "code" button to go to the plain text format[1]. In
all other cases (which I expect will be most) the doc will be easier
to read. Forcing plaintext, just because sometimes we might make a
mistake in the syntax seems like an overcorrection imho. Especially
because these docs are (hopefully) read more often than written.

[1]: https://github.com/postgres/postgres/blob/master/README.md?plain=1




Re: Improve EXPLAIN output for multicolumn B-Tree Index

2024-06-28 Thread Jelte Fennema-Nio
On Fri, 28 Jun 2024 at 00:41, Peter Geoghegan  wrote:
> Typically, no, it won't be. But there's really no telling for sure.
> The access patterns for a composite index on '(a, b)' with a qual
> "WHERE b = 5" are identical to a qual explicitly written "WHERE a =
> any() AND b = 5".

Hmm, that's true. But in that case the explain plan gives a clear hint
that something like that might be going on, because you'll see:

Index Cond: a = any() AND b = 5

That does make me think of another way, and maybe more "correct" way,
of representing Masahiros situation than adding a new "Skip Scan Cond"
row to the EXPLAIN output. We could explicitly include a comparison to
all prefix columns in the Index Cond:

Index Cond: ((test.id1 = 1) AND (test.id2 = ANY) AND (test.id3 = 101))

Or if you want to go with syntactically correct SQL we could do the following:

Index Cond: ((test.id1 = 1) AND ((test.id2 IS NULL) OR (test.id2 IS
NOT NULL)) AND (test.id3 = 101))

An additional benefit this provides is that you now know which
additional column you should use a more specific filter on to speed up
the query. In this case test.id2

OTOH, maybe it's not a great way because actually running that puts
the IS NULL+ IS NOT NULL query in the Filter clause (which honestly
surprises me because I had expected this "always true expression"
would have been optimized away by the planner).

> EXPLAIN (VERBOSE, ANALYZE) SELECT id1, id2, id3 FROM test WHERE id1 = 1 AND 
> (id2 IS NULL OR id2 IS NOT NULL) AND id3 = 101;
   QUERY PLAN
─
 Index Only Scan using test_idx on public.test  (cost=0.42..12809.10
rows=1 width=12) (actual time=0.027..11.234 rows=1 loops=1)
   Output: id1, id2, id3
   Index Cond: ((test.id1 = 1) AND (test.id3 = 101))
   Filter: ((test.id2 IS NULL) OR (test.id2 IS NOT NULL))

> What about cases where we legitimately have to vary our strategy
> during the same index scan?

Would my new suggestion above address this?

> In fact, that'd make sense even today, without skip scan (just with
> the 17 work on nbtree SAOP scans). Even with regular SAOP nbtree index
> scans, the number of primitive scans is hard to predict, and quite
> indicative of what's really going on with the scan.

*googles nbtree SAOP scans and finds the very helpful[1]*

Yes, I feel like this should definitely be part of the ANALYZE output.
Seeing how Lukas has to look at pg_stat_user_tables to get this
information seems quite annoying[2] and only possible on systems that
have no concurrent queries.

So it sounds like we'd want a "Primitive Index Scans" counter in
ANALYZE too. In addition to the number of filtered rows by, which if
we go with my proposal above should probably be called "Rows Removed
by Index Cond".

[1]: https://www.youtube.com/watch?v=jg2KeSB5DR8
[2]: https://youtu.be/jg2KeSB5DR8?t=188




Re: Improve EXPLAIN output for multicolumn B-Tree Index

2024-06-27 Thread Jelte Fennema-Nio
On Thu, 27 Jun 2024 at 22:02, Peter Geoghegan  wrote:
> It's also possible that we should just do something simple, like your
> patch, even though technically it won't really be accurate in cases
> where skip scan is used to good effect. Maybe showing the "default
> working assumption" about how the scan keys/clauses will behave at
> runtime is actually the right thing to do. Maybe I am just
> overthinking it.

IIUC, you're saying that your skip scan will improve the situation
Masahiro describes dramatically in some/most cases. But it still won't
be as good as a pure index "prefix" scan.

If that's the case then I do think you're overthinking this a bit.
Because then you'd still want to see this difference between the
prefix-scan keys and the skip-scan keys. I think the main thing that
the introduction of the skip scan changes is the name that we should
show, e.g. instead of "Non-key Filter" we might want to call it "Skip
Scan Cond"

I do think though that in addition to a "Skip Scan Filtered" count for
ANALYZE, it would be very nice to also get a "Skip Scan Skipped" count
(if that's possible to measure/estimate somehow). This would allow
users to determine how effective the skip scan was, i.e. were they
able to skip over large swaths of the index? Or did they skip over
nothing because the second column of the index (on which there was no
filter) was unique within the table




Re: ClientRead on ROLLABACK

2024-06-27 Thread Jelte Fennema-Nio
On Thu, 27 Jun 2024 at 17:21, Tom Lane  wrote:
> (We used to show "" in the query column in this state, but that
> was deemed less helpful than the current behavior.)

I think this is a super common confusion among users. Maybe we should
consider making it clearer that no query is currently being executed.
Something like

IDLE: last query: SELECT * FROM mytable;




Re: Support a wildcard in backtrace_functions

2024-06-27 Thread Jelte Fennema-Nio
On Wed, 15 May 2024 at 20:31, Robert Haas  wrote:
> That's fine, except that I think that what the previous discussion
> revealed is that we don't have consensus on how backtrace behavior
> ought to be controlled: backtrace_on_internal_error was one proposal,
> and this was a competing proposal, and neither one of them seemed to
> be completely satisfactory.

Attached is a rebased patchset of my previous proposal, including a
few changes that Michael preferred:
1. Renames log_backtrace to log_backtrace_mode
2. Rename internal to internal_error

I reread the thread since I previously posted the patch and apart from
Michaels feedback I don't think there was any more feedback on the
current proposal.

Rethinking about it myself though, I think the main downside of this
proposal is that if you want the previous behaviour of
backtrace_functions (add backtraces to all elog/ereports in the given
functions) you now need to set three GUCs:
log_backtrace_mode='all'
backtrace_functions='some_func'
backtrace_min_level=DEBUG5

The third one is not needed in the common case where someone only
cares about errors, but still needing to set log_backtrace_mode='all'
might seem a bit annoying. One way around that would be to make
log_backtrace_mode and backtrace_functions be additive instead of
subtractive.

Personally I think the proposed subtractive nature would be exactly
what I want for backtraces I'm interested in. Because I would want to
use backtrace_functions in this way:

1. I see an error I want a backtrace of: et log_backtrace_mode='all'
and try to trigger again.
2. Oops, there's many backtraces now let's filter by function: set
backtrace_functions=some_func

So if it's additive, I'd have to also undo log_backtrace_mode='all'
again at step 2. So changing two GUCs instead of one to do what I
want.
From 85a894cc32381a4ff2a1c7aab31476b2bbf6bdf3 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio 
Date: Thu, 21 Mar 2024 13:05:35 +0100
Subject: [PATCH 1/2] Add PGErrorVerbosity to typedefs.list

This one was missing, resulting in some strange alignment.
---
 src/include/utils/elog.h | 2 +-
 src/tools/pgindent/typedefs.list | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 054dd2bf62f..da1a7469fa5 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -493,7 +493,7 @@ typedef enum
 	PGERROR_TERSE,/* single-line error messages */
 	PGERROR_DEFAULT,			/* recommended style */
 	PGERROR_VERBOSE,			/* all the facts, ma'am */
-}			PGErrorVerbosity;
+} PGErrorVerbosity;
 
 extern PGDLLIMPORT int Log_error_verbosity;
 extern PGDLLIMPORT char *Log_line_prefix;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 61ad417cde6..67ec5408a98 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1778,6 +1778,7 @@ PGAsyncStatusType
 PGCALL2
 PGChecksummablePage
 PGContextVisibility
+PGErrorVerbosity
 PGEvent
 PGEventConnDestroy
 PGEventConnReset

base-commit: 3e53492aa7084bceaa92757c90e067d79768797e
-- 
2.34.1

From 9a1256aa3e4f585e3f588122662050f2302585dc Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio 
Date: Thu, 27 Jun 2024 10:56:10 +0200
Subject: [PATCH 2/2] Allow logging backtraces in more cases

We previously only had the backtrace_functions GUC to control when
backtraces were logged. Based on mailinglist discussion there were two
common cases that people wanted backtraces that were not covered by this
GUC though:

1. Logging backtraces for all internal errors
2. Logging backtraces for all errors

To support those two usecases, as well as to allow users to continue to
log backtraces for specific warnings using `backtrace_functions`, this
modifies the GUCs in the following way:

1. Adds a `log_backtrace` GUC, which can be set to `none` (default),
   `internal_error` and `all` to log different types of backtraces.
2. Change `backtrace_functions` to behave as an additional filter on top
   of `log_backtrace`. The empty string (the default) is now interpreted
   as doing no filtering based on function name.
3. Add a `backtrace_min_level` GUC, which limits for which log entries
   backtraces are written, based on their log level. This defaults to
   ERROR.

This does mean that setting `backtrace_functions=some_func` alone is not
enough to get backtraces for some_func. You now need to combine that
with `log_backtrace_mode=all` and if you want to get backtraces for
non-errors (which you previously got), you also need to change
backtrace_min_level to whichever level you want backtraces for.
---
 doc/src/sgml/config.sgml  | 82 +--
 src/backend/utils/error/elog.c| 30 ++-
 src/backend/utils/misc/guc_tables.c   | 50 +++
 src/backend/utils/misc/postgresql.conf.sample |  1 +
 src/include/utils

Re: doc: modify the comment in function libpqrcv_check_conninfo()

2024-06-27 Thread Jelte Fennema-Nio
On Thu, 27 Jun 2024 at 09:09, ikedarintarof
 wrote:
>
> Thank you for your comment!
>
> I've added the must_use_password argument. A new patch is attached.

s/whther/whether

nit: "it will do nothing" sounds a bit strange to me (but I'm not
native english). Something like this reads more natural to me:

an error. If must_use_password is true, the function raises an error
if no password is provided in the connection string. In any other case
it successfully completes.




Re: libpq: Fix lots of discrepancies in PQtrace

2024-06-27 Thread Jelte Fennema-Nio
On Thu, 27 Jun 2024 at 07:39, Michael Paquier  wrote:
> Looking at the whole, the rest of the patch set qualifies as a new
> feature, even if they're aimed at closing existing gaps.

Alright, seems reasonable. To be clear, I don't care at all about this
being backported personally.

> Particularly, you have bits of new infrastructure introduced in libpq
> like the current_auth_response business in 0004, making it a new
> feature by structure.
>
> +   conn->current_auth_response = AUTH_RESP_PASSWORD;
> ret = pqPacketSend(conn, PqMsg_PasswordMessage, pwd_to_send, 
> strlen(pwd_to_send) + 1);
> +   conn->current_auth_response = AUTH_RESP_NONE;
>
> It's a surprising approach.  Future callers of pqPacketSend() and
> pqPutMsgEnd() would easily miss that this flag should be set, as much
> as reset.  Isn't that something that should be added in input of these
> functions?

Yeah, I'm not entirely happy about it either. But adding an argument
to pqPutMsgEnd and pqPutPacketSend would mean all the existing call
sites would need to change, even though only 4 of them would care
about the new argument. You could argue that it's the better solution,
but it would at least greatly increase the size of the diff. Of course
to reduce the diff size you could make the old functions a wrapper
around a new one with the extra argument, but I couldn't think of a
good name for those functions. Overall I went for the chosen approach
here, because it only impacted code at the call sites for these auth
packets (which are the only v3 packets in the protocol that you cannot
interpret based on their contents alone).

I think your worry about easily missing to set/clear the flag is not a
huge problem in practice. We almost never add new authentication
messages and it's only needed there. Also the clearing is not even
strictly necessary for the tracing to behave correctly, but it seemed
like the right thing to do.

> +   AuthResponseType current_auth_response;
> I'd recommend to document what this flag is here for, with a comment.

Oops, yeah I forgot about that. Done now.


v2-0002-libpq-Add-suppress-argument-to-pqTraceOutputNchar.patch
Description: Binary data


v2-0003-libpq-Trace-StartupMessage-SSLRequest-GSSENCReque.patch
Description: Binary data


v2-0004-libpq-Trace-frontend-authentication-challenges.patch
Description: Binary data


v2-0005-libpq-Trace-responses-to-SSLRequest-and-GSSENCReq.patch
Description: Binary data


v2-0006-libpq-Trace-all-messages-received-from-the-server.patch
Description: Binary data


v2-0007-libpq-Trace-server-Authentication-messages-in-det.patch
Description: Binary data


v2-0008-libpq-Trace-NegotiateProtocolVersion-correctly.patch
Description: Binary data


Re: libpq: Fix lots of discrepancies in PQtrace

2024-06-26 Thread Jelte Fennema-Nio
On Wed, 26 Jun 2024 at 18:36, Alvaro Herrera  wrote:
> Thanks, Nathan.  I'm holding myself responsible for the rest ... will
> handle soon after the branch.

Sounds great. Out of curiosity, what is the backpatching policy for
something like this? Honestly most of these patches could be
considered bugfixes in PQtrace, so backpatching might make sense. OTOH
I don't think PQtrace is used very much in practice, so backpatching
might carry more risk than it's worth.




Re: doc: modify the comment in function libpqrcv_check_conninfo()

2024-06-26 Thread Jelte Fennema-Nio
On Wed, 26 Jun 2024 at 14:53, ikedarintarof
 wrote:
> The function 'libpqrcv_check_conninfo()' returns 'void', but the comment
> above says that the function returns true or false.
> I've attached a patch to modify the comment.

Agreed that the current comment is wrong, but the new comment should
mention the must_use_password argument. Because only if
must_use_password is true, will it throw an error.




Re: RFC: Additional Directory for Extensions

2024-06-26 Thread Jelte Fennema-Nio
On Tue, 25 Jun 2024 at 19:33, David E. Wheeler  wrote:
>
> On Jun 24, 2024, at 5:32 PM, Jelte Fennema-Nio  wrote:
>
> > Still, for the sake of completeness it might make sense to support
> > this whole list in extension_destdir. (assuming it's easy to do)
>
> It should be with the current patch, which just uses a prefix to paths in 
> `pg_config`.

Ah alright, I think it confused me because I never saw bindir being
used. But as it turns out the current backend code never uses bindir.
So that makes sense. I guess to actually use the binaries from the
extension_destdir/$BINDIR the operator needs to set PATH accordingly,
or the extension needs to be changed to support extension_destdir.

It might be nice to add a helper function to find binaries in BINDIR,
now that the resolution logic is more complex. Even if postgres itself
doesn't use it. That would make it easier for extensions to be
modified to support extension_distdir. Something like
find_bindir_executable(char *name)




  1   2   3   4   5   >