Re: Extension security improvement: Add support for extensions with an owned schema
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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?
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?
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?
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?
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
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
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
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
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
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
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
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
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
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
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.
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
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.
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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()
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
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
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
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
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
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
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
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
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()
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
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
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()
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
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)
