Re: Change GUC hashtable to use simplehash?
On Fri, Nov 17, 2023 at 11:02 AM Jeff Davis wrote: > > I had briefly experimented changing the hash table in guc.c to use > simplehash. It didn't offer any measurable speedup, but the API is > slightly nicer. > > I thought I'd post the patch in case others thought this was a good > direction or nice cleanup. This is not a comment on the patch itself, but since GUC operations are not typically considered performance or space sensitive, this comment from simplehash.h makes a case against it. * It's probably not worthwhile to generate such a specialized implementation * for hash tables that aren't performance or space sensitive. But your argument of a nicer API might make a case for the patch. I Best regards, Gurjeet http://Gurje.et
Re: simplehash: preserve consistency in case of OOM
On Fri, Nov 17, 2023 at 12:13 PM Andres Freund wrote: > > On 2023-11-17 10:42:54 -0800, Jeff Davis wrote: > > Right now, if allocation fails while growing a hashtable, it's left in > > an inconsistent state and can't be used again. +1 to the patch. > I'm not against allowing this - but I am curious, in which use cases is this > useful? I don't have an answer to that, but a guess would be when the server is dealing with memory pressure. In my view the patch has merit purely on the grounds of increasing robustness. > > @@ -446,10 +459,11 @@ SH_CREATE(MemoryContext ctx, uint32 nelements, void > > *private_data) > > /* increase nelements by fillfactor, want to store nelements elements > > */ > > size = Min((double) SH_MAX_SIZE, ((double) nelements) / > > SH_FILLFACTOR); > > > > - SH_COMPUTE_PARAMETERS(tb, size); > > + size = SH_COMPUTE_SIZE(size); > > > > - tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, > > sizeof(SH_ELEMENT_TYPE) * tb->size); > > + tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, > > sizeof(SH_ELEMENT_TYPE) * size); > > > > + SH_UPDATE_PARAMETERS(tb, size); > > return tb; > > } > > Maybe add a comment explaining why it's important to update parameters after > allocating? Perhaps something like this: + /* + * Update parameters _after_ allocation succeeds; prevent + * bogus/corrupted state. + */ + SH_UPDATE_PARAMETERS(tb, size); Best regards, Gurjeet http://Gurje.et
Replace references to malloc() in libpq documentation with generic language
The commit message in the attached patch provides the reasoning, as follows: The user does not benefit from knowing that libpq allocates some/all memory using malloc(). Mentioning malloc() here has a few downsides, and almost no benefits. All the relevant mentions of malloc() are followed by an explicit instruction to use PQfreemem() to free the resulting objects. So the docs perform the sufficient job of educating the user on how to properly free the memory. But these mentions of malloc() may still lead an inexperienced or careless user to (wrongly) believe that they may use free() to free the resulting memory. They will be in a lot of pain until they learn that (when linked statically) libpq's malloc()/free() cannot be mixed with malloc()/free() of whichever malloc() library the client application is being linked with. Another downside of explicitly mentioning that objects returned by libpq functions are allocated with malloc(), is that it exposes the implementation details of libpq to the users. Such details are best left unmentioned so that these can be freely changed in the future without having to worry about its effect on client applications. Whenever necessary, it is sufficient to tell the user that the objects/memory returned by libpq functions is allocated on the heap. That is just enough detail for the user to realize that the relevant object/memory needs to be freed; and the instructions that follow mention to use PQfreemem() to free such memory. One mention of malloc is left intact, because that mention is unrelated to how the memory is allocated, or how to free it. In passing, slightly improve the language of PQencryptPasswordConn() documentation. Best regards, Gurjeet http://Gurje.et v1-0001-Replace-references-to-malloc-in-libpq-documentati.patch Description: Binary data
Re: Remove extraneous break condition in logical slot advance function
On Fri, Oct 20, 2023 at 7:30 PM Bharath Rupireddy wrote: > > Hi, > > There exists an extraneous break condition in > pg_logical_replication_slot_advance(). When the end of WAL or moveto > LSN is reached, the main while condition helps to exit the loop, so no > separate break condition is needed. Attached patch removes it. > > Thoughts? +1 for the patch. The only advantage I see of the code as it stands right now is that it avoids one last call to CHECK_FOR_INTERRUPTS() by break'ing early. I don't think we'd lose much in terms of performance by making one (very cheap, in common case) extra call of this macro. Best regards, Gurjeet http://Gurje.et
Minor edit to src/bin/pg_upgrade/IMPLEMENTAION
The attached patch adds the word 'databases' to show that template0, template1 and postgres are databases in a default installation. Best regards, Gurjeet http://Gurje.et pg_upgrade.implementation.diff Description: Binary data
Re: [PoC/RFC] Multiple passwords, interval expirations
On Mon, Oct 9, 2023 at 2:31 AM Gurjeet Singh wrote:
>
> Next steps:
> - Break the patch into a series of smaller patches.
Please see attached the same v3 patch, but now split into 3 separate
patches. Each patch in the series depends on the previous patch to
have been applied. I have made sure that each patch passes `make
check` individually.
First patch adds the two new columns, rolsecondpassword and
rolsecondvaliduntil to the pg_authid shared catalog. This patch also
updates the corresponding pg_authid.dat file to set these values to
null for the rows populated during bootstrap. Finally, it adds code to
CreateRole() to set these columns' values to NULL for a role being
created.
The second patch updates the password extraction, verification
functions as well as authentication functions to honor the second
password, if any. There is more detailed description in the commit
message/body of the patch.
The third patch adds the SQL support to the ALTER ROLE command which
allows manipulation of both, the rolpassword and rolsecondpassword,
columns and their respective expiration timestamps,
rol[second]validuntil. This patch also adds regression tests for the
new SQL command, demonstrating the use of the new grammar.
v3-0001-Add-new-columns-to-pg_authid.patch
v3-0002-Update-password-verification-infrastructure-to-ha.patch
v3-0003-Added-SQL-support-for-ALTER-ROLE-to-manage-two-pa.patch
Best regards,
Gurjeet
http://Gurje.et
From bc7c35e53e421157c9425c198bc2557ad118a650 Mon Sep 17 00:00:00 2001
From: Gurjeet Singh
Date: Mon, 9 Oct 2023 11:36:05 -0700
Subject: [PATCH v3 1/3] Add new columns to pg_authid
Add two columns to pg_authid, namely rolsecondpassword and
rolsecondvaliduntil. These columns are added in preparation for the
password-rollover feature. These columns will store the hash and the
expiration time, repspectively, of a second password that the role can
use for login authentication.
---
src/backend/commands/user.c | 4 +++
src/include/catalog/pg_authid.dat | 45 ---
src/include/catalog/pg_authid.h | 2 ++
3 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index ce77a055e5..0afaf74865 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -452,9 +452,13 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
else
new_record_nulls[Anum_pg_authid_rolpassword - 1] = true;
+ new_record_nulls[Anum_pg_authid_rolsecondpassword - 1] = true;
+
new_record[Anum_pg_authid_rolvaliduntil - 1] = validUntil_datum;
new_record_nulls[Anum_pg_authid_rolvaliduntil - 1] = validUntil_null;
+ new_record_nulls[Anum_pg_authid_rolsecondvaliduntil - 1] = true;
+
new_record[Anum_pg_authid_rolbypassrls - 1] = BoolGetDatum(bypassrls);
/*
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 6b4a0d..b326e48376 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -23,76 +23,91 @@
rolname => 'POSTGRES', rolsuper => 't', rolinherit => 't',
rolcreaterole => 't', rolcreatedb => 't', rolcanlogin => 't',
rolreplication => 't', rolbypassrls => 't', rolconnlimit => '-1',
- rolpassword => '_null_', rolvaliduntil => '_null_' },
+ rolpassword => '_null_', rolvaliduntil => '_null_',
+ rolsecondpassword => '_null_', rolsecondvaliduntil => '_null_' },
{ oid => '6171', oid_symbol => 'ROLE_PG_DATABASE_OWNER',
rolname => 'pg_database_owner', rolsuper => 'f', rolinherit => 't',
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
- rolpassword => '_null_', rolvaliduntil => '_null_' },
+ rolpassword => '_null_', rolvaliduntil => '_null_',
+ rolsecondpassword => '_null_', rolsecondvaliduntil => '_null_' },
{ oid => '6181', oid_symbol => 'ROLE_PG_READ_ALL_DATA',
rolname => 'pg_read_all_data', rolsuper => 'f', rolinherit => 't',
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
- rolpassword => '_null_', rolvaliduntil => '_null_' },
+ rolpassword => '_null_', rolvaliduntil => '_null_',
+ rolsecondpassword => '_null_', rolsecondvaliduntil => '_null_' },
{ oid => '6182', oid_symbol => 'ROLE_PG_WRITE_ALL_DATA',
rolname => 'pg_write_all_data', rolsuper => 'f', rolinherit => 't',
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
- rolpassword => '_null_', rolvaliduntil => '_null_' },
+ rolpassword => '_null_', rolvaliduntil => '_null_',
+ rolsecondpassword => '_null_', rolsecondvaliduntil => '_null_' },
{ oid => '3373', oid_symbol => '
Re: [PoC/RFC] Multiple passwords, interval expirations
On Sun, Oct 8, 2023 at 1:01 PM Gurjeet Singh wrote: > > On Fri, Oct 6, 2023 at 1:29 PM Jeff Davis wrote: > > > > On Thu, 2023-10-05 at 14:28 -0700, Gurjeet Singh wrote: > > > > > This way there's a notion of a 'new' and 'old' passwords. > > > > IIUC, you are proposing that there are exactly two slots, NEW and OLD. > > When adding a password, OLD must be unset and it moves NEW to OLD, and > > adds the new password in NEW. DROP only works on OLD. Is that right? > > Yes, that's what I was proposing. But thinking a bit more about it, > the _implicit_ shuffling of labels 'new' and 'old' doesn't feel right > to me. The password that used to be referred to as 'new' now > automatically gets labeled 'old'. > > > It's close to the idea of deprecation, except that adding a new > > password implicitly deprecates the existing one. I'm not sure about > > that -- it could be confusing. > > +1 > I believe we should fix the _names_ of the slots the 2 passwords are > stored in, and provide commands that manipulate those slots by > respective names; the commands should not implicitly move the > passwords between the slots. Additionally, we may provide functions > that provide observability info for these slots. I propose the slot > names FIRST and SECOND (I picked these because these keywords/tokens > already exist in the grammar, but not yet sure if the grammar rules > would allow their use; feel free to propose better names). FIRST > refers to the the existing slot, namely rolpassword. SECOND refers to > the new slot we'd add, that is, a pgauthid column named > rolsecondpassword. The existing commands, or when neither FIRST nor > SECOND are specified, the commands apply to the existing slot, a.k.a. > FIRST. Please see attached the patch that implements this proposal. The patch is named password_rollover_v3.diff, breaking from the name 'multiple_passwords...', since this patch limits itself to address the password-rollover feature. The multiple_password* series of patches had removed a critical functionality, which I believe is crucial from security perspective. When a user does not exist, or has no passwords, or has passwords that have expired, we must pretend to perform authentication (network packet exchanges) as normally as possible, so that the absence of user, or lack of (or expiration of) passwords is not revealed to an attacker. I have restored the original behaviour in the CheckPWChallengeAuth() function; see commit aba99df407 [2]. I looked for any existing keywords that may better fit the purpose of naming the slots, better than FIRST and SECOND, but I could not find any. Instead of DROP to remove the passwords, I tried DELETE and the grammar/bison did not complain about it; so DELETE is an option, too, but I feel DROP FIRST/SECOND/ALL PASSWORD is a better companion of ADD FIRST/SECOND PASSWORD syntax, in the same vein as ADD/DROP COLUMN. The doc changes are still missing, but the regression tests and the comments therein should provide a good idea of the user interface of this new feature. Documenting this behaviour in a succinct manner feels difficult; so ideas welcome for how to inform the reader that now a role is accompanied by two slots to store the passwords, and that the old commands operate on the first slot, and to operate on the second password slot one must use the new syntax. I guess it would be best to start the relevant section with "To support gradual password rollovers, Postgres provides the ability to store 2 active passwords at the same time. The passwords are referred to as FIRST and SECOND password. Each of these passwords can be changed independently, and each of these can have an associated expiration time, if necessary." Since these new commands are only available to ALTER ROLE (and not to CREATE ROLE), the corresponding command doc page also needs to be updated. Next steps: - Break the patch into a series of smaller patches. - Add TAP tests (test the ability to actually login with these passwords) - Add/update documentation - Add more regression tests The patch progress can be followed on the Git branch password_rollover_v3 [1]. This branch begins uses multiple_passwords_v4 as starting point, and removes unnecessary code (commit 326f60225f [3]) The v1 (and tombstone of v2) patches of password_rollover never finished as the consensus changed while they were in progress, but they exist as sibling branches of the v3 branch. [1]: https://github.com/gurjeet/postgres/commits/password_rollover_v3 [2]: https://github.com/gurjeet/postgres/commit/aba99df407a523357db2813f0eea0b45dbeb6006 [3]: https://github.com/gurjeet/postgres/commit/326f60225f0e660338fc9c276c8728dc10db435b Best regards, Gurjeet http://Gurje.et password_rollover_v3.diff Description: Binary data
Re: [PoC/RFC] Multiple passwords, interval expirations
On Fri, Oct 6, 2023 at 1:29 PM Jeff Davis wrote:
>
> On Thu, 2023-10-05 at 14:28 -0700, Gurjeet Singh wrote:
>
> > This way there's a notion of a 'new' and 'old' passwords.
>
> IIUC, you are proposing that there are exactly two slots, NEW and OLD.
> When adding a password, OLD must be unset and it moves NEW to OLD, and
> adds the new password in NEW. DROP only works on OLD. Is that right?
Yes, that's what I was proposing. But thinking a bit more about it,
the _implicit_ shuffling of labels 'new' and 'old' doesn't feel right
to me. The password that used to be referred to as 'new' now
automatically gets labeled 'old'.
> It's close to the idea of deprecation, except that adding a new
> password implicitly deprecates the existing one. I'm not sure about
> that -- it could be confusing.
+1
> We could also try using a verb like "expire" that could be coupled with
> a date, and that way all old passwords would always have some validity
> period.
Forcing the users to pick an expiry date for a password they intend to
rollover, when an expiry date did not exist before for that password,
feels like adding more burden to their password rollover decision
making. The dates and rules of password rollover may be a part of a
system external to their database, (wiki, docs, calendar, etc.) which
now they will be forced to translate into a timestamp to specify in
the rollover commands.
I believe we should fix the _names_ of the slots the 2 passwords are
stored in, and provide commands that manipulate those slots by
respective names; the commands should not implicitly move the
passwords between the slots. Additionally, we may provide functions
that provide observability info for these slots. I propose the slot
names FIRST and SECOND (I picked these because these keywords/tokens
already exist in the grammar, but not yet sure if the grammar rules
would allow their use; feel free to propose better names). FIRST
refers to the the existing slot, namely rolpassword. SECOND refers to
the new slot we'd add, that is, a pgauthid column named
rolsecondpassword. The existing commands, or when neither FIRST nor
SECOND are specified, the commands apply to the existing slot, a.k.a.
FIRST.
The user interface might look like the following:
-- Create a user, as usual
CREATE ROLE u1 PASSWORD 'p1' VALID UNTIL '2020/01/01';
-- This automatically occupies the 'first' slot
-- Add another password that the user can use for authentication.
ALTER ROLE u1 ADD SECOND PASSWORD 'p2' VALID UNTIL '2021/01/01';
-- Change both the passwords' validity independently; this solves the
-- problem with the previous '2-element stack' approach, where we
-- could not address the password at the bottom of the stack.
ALTER ROLE u1 SECOND PASSWORD VALID UNTIL '2022/01/01';
ALTER ROLE u1 [ [ FIRST ] PASSWORD ] VALID UNTIL '2022/01/01';
-- If, for some reason, the user wants to get rid of the latest password added.
ALTER ROLE u1 DROP SECOND PASSWORD;
-- Add a new password (p3) in 'second' slot
ALTER ROLE u1 ADD SECOND PASSWORD 'p3' VALID UNTIL '2023/01/01';
-- Attempting to add a password while the respective slot is occupied
-- results in error
ALTER ROLE u1 ADD [ [ FIRST ] PASSWORD ] 'p4' VALID UNTIL '2024/01/01';
ERROR: first password already exists
ALTER ROLE u1 ADD SECOND PASSWORD 'p4' VALID UNTIL '2024/01/01';
ERROR: second password already exists
-- Users can use this function to check whether a password slot is occupied
=> select password_exists('u1', 'first');
password_exists
-
t
=> select password_exists('u1', 'second');
password_exists
-
t
-- Remove all passwords from the role. Both, 'first' and 'second',
passwords are removed.
ALTER ROLE u1 DROP ALL PASSWORD;
Best regards,
Gurjeet
http://Gurje.et
Re: [PoC/RFC] Multiple passwords, interval expirations
On Sun, Oct 8, 2023 at 10:29 AM Bruce Momjian wrote: > > I was speaking of autoremoving in cases where we are creating a new one, > and taking the previous new one and making it the old one, if that was > not clear. Yes, I think I understood it differently. I understood it to mean that this behaviour would apply to all passwords, those created by existing commands, as well as to those created by new commands for rollover use case. Whereas you meant this autoremove behaviour to apply only to those passwords created by/for rollover related commands. I hope I've understood your proposal correctly this time around :-) I believe the passwords created by rollover feature should behave by the same rules as the rules for passwords created by existing CREATE/ALTER ROLE commands. If we implement the behaviour to delete expired passwords, then I believe that behaviour should apply to all passwords, irrespective of which command/feature was used to create a password. Best regards, Gurjeet http://Gurje.et
Re: [PoC/RFC] Multiple passwords, interval expirations
On Fri, Oct 6, 2023 at 1:46 PM Bruce Momjian wrote: > > On Fri, Oct 6, 2023 at 01:20:03PM -0700, Jeff Davis wrote: > > The basic problem, as I see it, is: how do we keep users from > > accidentally dropping the wrong password? Generated unique names or > > I thought we could auto-remove old password if the valid-until date is > in the past. Autoremoving expired passwords will surprise users, and not in a good way. Making a password, even an expired one, disappear from the system will lead to astonishment. Among uses of an expired password are cases of it acting like a tombstone, and the case where the user may want to extend the validity of a password, instead of having to create a new one and change application configuration(s) to specify the new password. Best regards, Gurjeet http://Gurje.et
Re: [PoC/RFC] Multiple passwords, interval expirations
On Thu, Oct 5, 2023 at 1:09 PM Jeff Davis wrote: > > > > > I think this would be mighty confusing to users since it's not clear > > that > > adding a password will potentially invalidate a current password > > (which > > might be actively in use), but only if there are already 2 in the > > stack. I > > worry that such a desіgn might be too closely tailored to the > > implementation details. If we proceed with this design, perhaps we > > should > > consider ERROR-ing if a user tries to add a third password. > > I agree that the proposed language is confusing, especially because ADD > causes a password to be added and another one to be removed. But > perhaps there are some non-confusing ways to expose a similar idea. How about a language like the following (I haven't tried if this will work in the grammar we have): CREATE ROLE u1 PASSWORD 'p1'; ALTER ROLE u1ADD NEW PASSWORD 'p2'; ALTER ROLE u1 ADD NEW PASSOWRD 'p3'; ERROR: Cannot have more than 2 passwords at the same time. ALTER ROLE u1 DROP OLD PASSWORD; ALTER ROLE u1 ADD NEW PASSOWRD 'p3'; -- succeeds; forgets password 'p1'; p2 and p3 can be used to login ALTER ROLE u1 DROP NEW PASSWORD; -- forgets password 'p3'. Only 'p2' can be used to login ALTER ROLE u1 ADD NEW PASSOWRD 'p4'; -- succeeds; 'p2' and 'p4' can be used to login -- Set the valid-until of 'new' (p4) password ALTER ROLE u1 VALID UNTIL '2024/01/01'; -- If we need the ability to change valid-until of both old and new, we may allow something like the following. ALTER ROLE u1 [_NEW_ | OLD] VALID UNTIL '2024/01/01'; This way there's a notion of a 'new' and 'old' passwords. User cannot add a third password without explicitly dropping one of existing passwords (either old or new). At any time the user can choose to drop the old or the new password. Adding a new password will mark the current password as 'old'; if there's only old password (because 'new' was dropped) the 'old' password will remain intact and the new one will be placed in 'current'/new spot. So in normal course of operation, even for automated jobs, the expected flow to roll over the passwords would be: ALTER USER u1 DROP OLD PASSWORD; -- success if there is an old password; otherwise NOTICE: no old password ALTER USER u1 ADD NEW PASSWORD 'new-secret'; > The thing I like about Gurjeet's proposal is that it's well-targeted at > a specific use case rather than trying to be too general. That makes it > a little easier to avoid certain problems like having a process that > adds passwords and never removes the old ones (leading to weird > problems like 47000 passwords for one user). > > But it also feels strange to be limited to two -- perhaps the password > rotation schedule or policy just doesn't work with a limit of two, or > perhaps that introduces new kinds of mistakes. > > Another idea: what if we introduce the notion of deprecating a > password? I'll have to think more about it, but perhaps my above proposal addresses the use case you describe. > if we allow multiple deprecated passwords, we'd still have to come up > with some way to address them (names, numbers, validity period, > something). But by isolating the problem to deprecated passwords only, > it feels like the system is still being restored to a clean state with > at most one single current password. The awkwardness is contained to > old passwords which will hopefully go away soon anyway and not > represent permanent clutter. +1 Best regards, Gurjeet http://Gurje.et
Re: [PoC/RFC] Multiple passwords, interval expirations
On Thu, Oct 5, 2023 at 12:04 PM Nathan Bossart wrote: > > On Wed, Oct 04, 2023 at 10:41:15PM -0700, Gurjeet Singh wrote: > > The patches posted in this thread so far attempt to add the ability to > > allow the user to have an arbitrary number of passwords. I believe > > that allowing arbitrary number of passwords is not only unnecessary, > > but the need to name passwords, the need to store them in a shared > > catalog, etc. may actually create problems in the field. The > > users/admins will have to choose names for passwords, which they > > didn't have to previously. The need to name them may also lead to > > users storing password-hints in the password names (e.g. 'mom''s > > birthday', 'ex''s phone number', 'third password'), rendering the > > passwords weak. > > > > Moreover, allowing an arbitrarily many number of passwords will > > require us to provide additional infrastructure to solve problems like > > observability (which passwords are currently in use, and which ones > > have been effectively forgotten by applications), or create a nuisance > > for admins that can create more problems than it solves. > > IMHO neither of these problems seems insurmountable. Agreed. > Besides advising > against using hints as names, we could also automatically generate safe > names, or even disallow user-provided names entirely. Somehow naming passwords doesn't feel palatable to me. > And adding > observability for passwords seems worthwhile anyway. Agreed. > > So I propose that the feature should allow no more than 2 passwords > > for a role, each with their own validity periods. This eliminates the > > need to name passwords, because at any given time there are no more > > than 2 passwords; current one, and old one. This also eliminates the > > need for a shared catalog to hold passwords, because with the limit of > > 2 imposed, we can store the old password and its validity period in > > additional columns in the pg_authid table. > > Another approach could be to allow an abritrary number of passwords but to > also allow administrators to limit how many passwords can be associated to > each role. That way, we needn't restrict this feature to 2 passwords for > everyone. Perhaps 2 should be the default, but in any case, IMO we > shouldn't design to only support 2. I don't see a real use case to support more than 2 passwords. Allowing an arbitrary number of passwords might look good and clean from aesthetics and documentation perspective (no artificially enforced limits, as in the zero-one-infinity rule), but in absence of real use cases for that many passwords, I'm afraid we might end up with a feature that creates more and worse problems for the users than it solves. > > In essence, we create a stack that can hold 2 passwords. Pushing an > > element when it's full will make it forget the bottom element. Popping > > the stack makes it forget the top element, and the only remaining > > element, if any, becomes the top. > > I think this would be mighty confusing to users since it's not clear that > adding a password will potentially invalidate a current password (which > might be actively in use), but only if there are already 2 in the stack. Fair point. We can aid the user by emitting a NOTICE (or a WARNING) message whenever an old password is removed from the system because of addition of a new password. > I > worry that such a desіgn might be too closely tailored to the > implementation details. If we proceed with this design, perhaps we should > consider ERROR-ing if a user tries to add a third password. I did not have a stack in mind when developing the use case and the grammar, so implementation details did not drive this design. This new design was more of a response to the manageability nightmare that I could see the old approach may lead to. When writing the email I thought mentioning the stack analogy may make it easier to develop a mental model. I certainly won't suggest using it in the docs for explanation :-) > > -- If, for some reason, the user wants to get rid of the latest password > > added. > > -- Remove 'p2' (current password). The old password (p1), will be restored > > to > > -- rolpassword, along with its valid-until value. > > ALTER ROLE u1 PASSWORD NULL; > > -- This may come as a surprise to some users, because currently they expect > > the > > -- user to completely lose the ability to use passwords for login after this > > -- command. To get the old behavior, the user must now use the ALL PASSWORD > > -- NULL incantation; see below. > > -- Issuing this command one more time will remove even the restored > > password, > > -- hence leaving the user with no pass
Re: Good News Everyone! + feature proposal
On Thu, Oct 5, 2023 at 1:52 AM Jon Erdman wrote: > > So I have some good news! At long last I've found a company/manager that > wants to actually factually pay me to do some work on PG!! Congratulations! > For the proposal (this one is a bit Apple specific): because my team > offers managed postgres to our Apple-internal customers, many of whom > are not database experts, or at least not postgres experts, we'd like to > be able to toggle the availability of UNLOGGED tables in > postgresql.conf, so our less clueful users have fewer footguns. > > So, my proposal is for a GUC to implement that, which would *OF COURSE* > undefault to allowing UNLOGGED. That was difficult to parse at first glance. I guess you mean the GUC's default value will not change the current behaviour, as you mention below. > The reasoning here is we have autofailover set up for our standard > cluster offering that we give to customers, using sync-rep to guarantee > no data loss if we flop to the HA node. Any users not up to speed on > what UNLOGGED really means could inadvertently lose data, so we'd like > to be able to force it to be off, and turn it on upon request after > educating the customer in question it's it's a valid use case. > > So to begin with: I'm sure some folks will hate this idea, but maybe a > good many wont, and remember, this would default to UNLOGGED enabled, so > no change to current behaviour. and no encouragement to disallow it, but > just the ability to do so, which i think is useful in > hosted/managed/multi-tenant environment where most things are taken care > of for the customer. I see the need to disable this feature and agree that some installations may need it, where the users are not savvy enough to realize its dangers and fall for its promise to increase INSERT/UPDATE performance. Your specific example of an internal hosted/managed service is a good example. Even in smaller installations the DBA might want to disable this, so that unwary developers don't willy-nilly create unlogged tables and end up losing data after a failover. I hope others can provide more examples and situations where this may be useful, to make it obvious that we need this feature. My first reaction was to make it a GRANTable permission. But since one can always do `ALTER USER savvy_user SET allow_unlogged_tables TO true` and override the system-wide setting in postgresql.conf, for a specific user, I feel a GUC would be the right way to implement it. The complaint about too many GUCs is a valid one, but I'd worry more about it if it were an optimizer/performance improvement being hidden behind a GUC. This new GUC would be a on-off switch to override the SQL/grammar feature provided by the UNLOGGED keyword, hence not really a concern IMO. > So I'd like to get a general idea how likely this would be to getting > accepted if it did it, and did it right? Like others said, there are no guarantees. A working patch may help guide people's opinion one way or the other, so I'd work on submitting a patch while (some) people are in agreement. > Let the flame war begin! Heh. I'm sure you already know this, but this community's flame wars are way more timid compared to what members of other communities may be used to :-) I consider it lucky if someone throws as much as a lit match. > PS: I'm SO happy that this phase of my postgres journey has finally > started I, too, am very happy for you! :-) > Jon Erdman (aka StuckMojo on IRC) TIL. I wish there was a directory of IRC identities that pointed to real identities (at least for folks who don't mind this mapping available in the open), so that when we converse in IRC, we have a face to go with the IRC handles. As a human I feel that necessity. Best regards, Gurjeet http://Gurje.et
Re: [PoC/RFC] Multiple passwords, interval expirations
I had an idea to simplify this feature/patch and after some validation in internal discussions, I am posting the new approach here. I'd appreciate any feedback and comments. To begin with, the feature we are chasing is to make it convenient for the users to rollover their passwords. Currently there is no easy way to rollover passwords without increasing the risk of an application outage. After a password change, the users/admins have to rush to change the password in all locations where it is stored. There is a window of time where if the application password is not changed to the new one, and the application tries to connect/reconnect for any reason, the application will fail authentication and lead to an outage. I personally haven't seen any attempts by any application/driver/framework to solve this problem in the wild, so following is just me theorizing how one may solve this problem on the application side; there may be other ways in which others may solve this problem. The application may be written in such a way that upon password authentication failure, it tries again with a second password. The application's configuration file (or environment variables) may allow specifying 2 passwords at the same time, and the application will keep trying these 2 passwords alternatively until it succeeds or the user restarts it with a new configuration. With such a logic in place in their application, the users may first change the configuration of all the instances of the application to hold the new password along with the old/current working password, and only then change the password in the database. This way, in the event of an application instance start/restart either the old password will succeed, or the new password will. There may be other ways to solve this problem, but I can't imagine any of those ways to be convenient and straightforward. At least not as convenient as it can be if the database itself allowed for storing both the passwords, and honored both passwords at the same time, while allowing to associate a separate validity period with each of the passwords. The patches posted in this thread so far attempt to add the ability to allow the user to have an arbitrary number of passwords. I believe that allowing arbitrary number of passwords is not only unnecessary, but the need to name passwords, the need to store them in a shared catalog, etc. may actually create problems in the field. The users/admins will have to choose names for passwords, which they didn't have to previously. The need to name them may also lead to users storing password-hints in the password names (e.g. 'mom''s birthday', 'ex''s phone number', 'third password'), rendering the passwords weak. Moreover, allowing an arbitrarily many number of passwords will require us to provide additional infrastructure to solve problems like observability (which passwords are currently in use, and which ones have been effectively forgotten by applications), or create a nuisance for admins that can create more problems than it solves. So I propose that the feature should allow no more than 2 passwords for a role, each with their own validity periods. This eliminates the need to name passwords, because at any given time there are no more than 2 passwords; current one, and old one. This also eliminates the need for a shared catalog to hold passwords, because with the limit of 2 imposed, we can store the old password and its validity period in additional columns in the pg_authid table. The patches so far also add a notion of max validity period of passwords, which only a superuser can override. I believe this is a useful feature, but that feature can be dealt with separately, independent of password rollover feature. So in the newer patches I will not include the relevant GUC and code. With the above being said, following is the user interface I can think of that can allow for various operations that users may need to perform to rollover their passwords. The 'ADD PASSWORD' and 'ALL PASSWORD' are additions to the grammar. rololdpassword and rololdvaliduntil will be new columns in pg_authid that will hold the old password and its valid-until value. In essence, we create a stack that can hold 2 passwords. Pushing an element when it's full will make it forget the bottom element. Popping the stack makes it forget the top element, and the only remaining element, if any, becomes the top. -- Create a user, as usual CREATE ROLE u1 PASSWORD 'p1' VALID UNTIL '2020/01/01'; -- Add another password that the user can use for authentication. This moves -- the 'p1' password hash and its valid-until value to rololdpassword and -- rololdvaliduntil, respectively. ALTER ROLE u1 ADD PASSWORD 'p2' VALID UNTIL '2021/01/01'; -- Change the password 'p2's (current password's) validity ALTER ROLE u1 VALID UNTIL '2022/01/01'; -- Note that currently I don't have a proposal for how to change the old -- password's validity period, without deleting the latest/main
Re: Document that server will start even if it's unable to open some TCP/IP ports
On Tue, Sep 26, 2023 at 4:02 PM Bruce Momjian wrote: > > Patch applied back to PG 11. +Peter E. since I received the following automated note: > Closed in commitfest 2023-09 with status: Moved to next CF (petere) Just a note that this patch has been committed (3fea854691), so I have marked the CF item [1] as 'Committed', and specified Bruce as the committer. [1]: https://commitfest.postgresql.org/45/4333/ Best regards, Gurjeet http://Gurje.et
Re: Document that server will start even if it's unable to open some TCP/IP ports
On Fri, Sep 8, 2023 at 7:52 AM Bruce Momjian wrote: > > On Thu, Sep 7, 2023 at 09:21:07PM -0700, Nathan Bossart wrote: > > On Thu, Sep 07, 2023 at 07:13:44PM -0400, Bruce Momjian wrote: > > > On Thu, Sep 7, 2023 at 02:54:13PM -0700, Nathan Bossart wrote: > > >> IMO the phrase "open a port" is kind of nonstandard. I think we should > > >> say > > >> something along the lines of > > >> > > >>If listen_addresses is not empty, the server will start only if it can > > >>listen on at least one of the specified addresses. A warning will be > > >>emitted for any addresses that the server cannot listen on. > > > > > > Good idea, updated patch attached. > > > > I still think we should say "listen on an address" instead of "open a > > port," but otherwise it LGTM. > > Agreed, I never liked the "port" mention. I couldn't figure how to get > "open" out of the warning sentence though. Updated patch attached. LGTM. Best regards, Gurjeet http://Gurje.et
psql --no-connect option
Currently, psql exits if a database connection is not established when psql is launched. Sometimes it may be useful to launch psql without connecting to the database. For example, a user may choose to start psql and then pipe commands via stdin, one of which may eventually perform the \connect command. Or the user may be interested in performing operations that psql can perform, like setting variables etc., before optionally initiating a connection. The attached patch introduces the --no-connect option, which allows psql to continue operation in absence of connection options. This patch is nowhere close to finished, but I'm posting it here to gauge interest in the feature. For example, this patch results in an undesirable output (0.0 server version), as seen below. $ psql --no-connect psql (17devel, server 0.0.0) WARNING: psql major version 17, server major version 0.0. Some psql features might not work. Type "help" for help. !?> The patch needs many improvements, like not expecting to inherit connection options from a previous connection, etc., but mostly in identifying the conflicting options; an example of this is included in the patch where psql throws an error if --no-connect and --list are specified together. $ psql --no-connect --list psql: error: --no-connect cannot be specified with --list Best regards, Gurjeet http://Gurje.et psql_no_connect.diff Description: Binary data
Re: Doc limitation update proposal: include out-of-line OID usage per TOAST-ed columns
On Wed, Apr 26, 2023 at 4:48 AM David Rowley wrote: > > On Sun, 23 Apr 2023, 3:42 am Gurjeet Singh, wrote: >> >> I anticipate that edits to Appendix K Postgres Limits will prompt >> improving the note in there about the maximum column limit, That note >> is too wordy, and sometimes confusing, especially for the audience >> that it's written for: newcomers to Postgres ecosystem. > > > I doubt it, but feel free to submit a patch yourself which improves it > without losing the information which the paragraph is trying to convey. I could not think of a way to reduce the wordiness without losing information. But since this page is usually consulted by those who are new to Postgres, usually sent here by a search engine, I believe the page can be improved for that audience, without losing much in terms of accuracy. I agree the information provided in the paragraph about max-columns is pertinent. But since the limits section is most often consulted by people migrating from other database systems (hence the claim that they're new to the Postgres ecosystem), I imagine the terminology used there may cause confusion for the reader. So my suggestion is to make that paragraph, and perhaps even that page, use fewer hacker/internals terms. Technically, there may be a difference between table vs. relation, row vs. tuple, and column vs. field. But using those terms, seemingly interchangeably on that page does not help the reader. The page neither describes the terms, nor links to their definitions, so a reader is left with more questions than before. For example, > rows per table:: limited by the number of tuples that can fit onto > 4,294,967,295 pages A newcomer> what's a tuple in this context, and how is it similar to/different from a row? Please see attached the proposed patch, which attempts to make that language newcomer-friendly. The patch adds one link for TOAST, and replaces Postgres-specific terms with generic ones. PS: I've retained line boundaries, so that `git diff --color-words doc/src/sgml/limits.sgml` would make it easy to see the changes. Best regards, Gurjeet http://Gurje.et limits-generic-terms.diff Description: Binary data
Re: There should be a way to use the force flag when restoring databases
On Sun, Jul 23, 2023 at 6:09 AM Ahmed Ibrahim
wrote:
>
> Hi everyone,
>
> I have been working on this. This is a proposed patch for it so we have a
> force option for DROPping the database.
>
> I'd appreciate it if anyone can review.
Hi Ahmed,
Thanks for working on this patch!
+
+int force;
That extra blank line is unnecessary.
Using the bool data type, instead of int, for this option would've
more natural.
+if (ropt->force){
Postgres coding style is to place the curly braces on a new line,
by themselves.
+char *dropStmt = pg_strdup(te->dropStmt);
See if you can use pnstrdup(). Using that may obviate the need for
doing the null-placement acrobatics below.
+PQExpBuffer ftStmt = createPQExpBuffer();
What does the 'ft' stand for in this variable's name?
+dropStmt[strlen(dropStmt) - 2] = ' ';
+dropStmt[strlen(dropStmt) - 1] = '\0';
Try to evaluate the strlen() once and reuse it.
+appendPQExpBufferStr(ftStmt, dropStmt);
+appendPQExpBufferStr(ftStmt, "WITH(FORCE);");
+te->dropStmt = ftStmt->data;
+}
+
Remove the extra trailing whitespace on that last blank line.
I think this whole code block needs to be protected by an 'if
(ropt->createDB)' check, like it's done about 20 lines above this
hunk. Otherwise, we may be appending 'WITH (FORCE)' for the DROP
command of a different (not a database) object type.
Also, you may want to check that the target database version is
one that supports WITH force option. This command will fail for
anything before v13.
The patch needs doc changes (pg_restore.sgml). And it needs to
mention --force option in the help output, as well (usage() function).
Can you please see if you can add appropriate test case for this.
The committers may insist on it, when reviewing.
Here are a couple of helpful links on how to prepare and submit
patches to the community. You may not need to strictly adhere to
these, but try to pick up a few recommendations that would make the
reviewer's job a bit easier.
[1]: https://wiki.postgresql.org/wiki/Creating_Clean_Patches
[2]: https://wiki.postgresql.org/wiki/Submitting_a_Patch
Best regards,
Gurjeet
http://Gurje.et
Re: MERGE ... RETURNING
On Fri, Jul 21, 2023 at 7:17 PM Dean Rasheed wrote:
>
> On Mon, 17 Jul 2023 at 20:43, Jeff Davis wrote:
> >
> > > > Maybe instead of a function it could be a special table reference
> > > > like:
> > > >
> > > > MERGE ... RETURNING MERGE.action, MERGE.action_number, id, val?
> > > >
> > The benefits are:
> >
> > 1. It is naturally constrained to the right context. It doesn't require
> > global variables and the PG_TRY/PG_FINALLY, and can't be called in the
> > wrong contexts (like SELECT).
> >
> > 2. More likely to be consistent with eventual support for NEW/OLD
> > (actually BEFORE/AFTER for reasons the prior thread discussed).
> >
>
> Thinking about this some more, I think that the point about
> constraining these functions to the right context is a reasonable one,
> and earlier versions of this patch did that better, without needing
> global variables or a PG_TRY/PG_FINALLY block.
>
> Here is an updated patch that goes back to doing it that way. This is
> more like the way that aggregate functions and GROUPING() work, in
> that the parser constrains the location from which the functions can
> be used, and at execution time, the functions rely on the relevant
> context being passed via the FunctionCallInfo context.
>
> It's still possible to use these functions in subqueries in the
> RETURNING list, but attempting to use them anywhere else (like a
> SELECT on its own) will raise an error at parse time. If they do
> somehow get invoked in a non-MERGE context, they will elog an error
> (again, just like aggregate functions), because that's a "shouldn't
> happen" error.
>
> This does nothing to be consistent with eventual support for
> BEFORE/AFTER, but I think that's really an entirely separate thing,
+1
> From a user perspective, writing something like "BEFORE.id" is quite
> natural, because it's clear that "id" is a column, and "BEFORE" is the
> old state of the table. Writing something like "MERGE.action" seems a
> lot more counter-intuitive, because "action" isn't a column of
> anything (and if it was, I think this syntax would potentially cause
> even more confusion).
>
> So really, I think "MERGE.action" is an abuse of the syntax,
> inconsistent with any other SQL syntax, and using functions is much
> more natural, akin to GROUPING(), for example.
There seems to be other use cases which need us to invent a method to
expose a command-specific alias. See Tatsuo Ishii's call for help in
his patch for Row Pattern Recognition [1].
I was not able to find a way to implement expressions like START.price
(START is not a table alias). Any suggestion is greatly appreciated.
It looks like the SQL standard has started using more of such
context-specific keywords, and I'd expect such keywords to only
increase in future, as the SQL committee tries to introduce more
features into the standard.
So if MERGE.action is not to your taste, perhaps you/someone can
suggest an alternative that doesn't cause a confusion, and yet
implementing it would open up the way for more such context-specific
keywords.
I performed the review vo v9 patch by comparing it to v8 and v7
patches, and then comparing to HEAD.
The v9 patch is more or less a complete revert to v7 patch, plus the
planner support for calling the merge-support functions in subqueries,
parser catching use of merge-support functions outside MERGE command,
and name change for one of the support functions.
But reverting to v7 also means that some of my gripes with that
version also return; e.g. invention of EXPR_KIND_MERGE_RETURNING. And
as noted in v7 review, I don't have a better proposal.
Function name changed from pg_merge_when_clause() to
pg_merge_when_clause_number(). That's better, even though it's a bit
of mouthful.
Doc changes (compared to v7) look good.
The changes made to tests (compared to v7) are for the better.
- * Uplevel PlaceHolderVars and aggregates are replaced, too.
+ * Uplevel PlaceHolderVars, aggregates, GROUPING() expressions and merge
+ * support functions are replaced, too.
Needs Oxford comma: s/GROUPING() expressions and/GROUPING() expressions, and/
+pg_merge_action(PG_FUNCTION_ARGS)
+{
...
+relaction = mtstate->mt_merge_action;
+if (relaction)
+{
..
+}
+
+PG_RETURN_NULL();
+}
Under what circumstances would the relaction be null? Is it okay to
return NULL from this function if relaction is null, or is it better
to throw an error? These questions apply to the
pg_merge_when_clause_number() function as well.
[1]: Row pattern recognition
https://www.postgresql.org/message-id/flat/20230625.210509.1276733411677577841.t-ishii%40sranhm.sra.co.jp
Best regards,
Gurjeet
http://Gurje.et
Re: psql not responding to SIGINT upon db reconnection
On Mon, Jul 24, 2023 at 9:26 AM Tristan Partin wrote: > attached patch +/* + * Restore the default SIGINT behavior while within libpq. Otherwise, we + * can never exit from polling for the database connection. Failure to + * restore is non-fatal. + */ +newact.sa_handler = SIG_DFL; +rc = sigaction(SIGINT, , ); There's no action taken if rc != 0. It doesn't seem right to continue as if everything's fine when the handler registration fails. At least a warning is warranted, so that the user reports such failures to the community. Best regards, Gurjeet http://Gurje.et
Re: Issue in _bt_getrootheight
On Fri, Jul 21, 2023 at 10:42 AM Tom Lane wrote: > > Gurjeet Singh writes: > > Please see attached the patch that does this. Let me know if this patch > > helps. > > I don't like this patch one bit, because it adds a lot of overhead > (i.e., an extra index_open/close cycle for every btree index in every > query) to support a tiny minority use-case. I anticipated the patch's performance impact may be a concern, but before addressing it I wanted to see if the patch actually helped Index Adviser. Ahmed has confirmed that my proposed patch works for him. I believe the additional index_open() would not affect the performance significantly, since the very same indexes were index_open()ed just before calling the get_relation_info_hook. All the relevant caches would be quite fresh because of the index_open() in the same function above. And since the locks taken on these indexes haven't been released, we don't have to work hard to take any new locks (hence the index_open() with NoLock flag). > How come we don't > already know whether the index is hypothetical at the point where > _bt_getrootheight is called now? Because the 'hypthetical' flag is not stored in catalogs, and that's okay; see below. At that point, the only indication that an index may be a hypothetical index is if RelationGetNumberOfBlocks() returns 0 for it, and that's what Ahmed's proposed patch relied on. But I think extrapolating that info->pages==0 implies it's a hypothetical index, is stretching that assumption too far. > Actually, looking at the existing comment at the call site: > > /* > * Allow a plugin to editorialize on the info we obtained from the > * catalogs. Actions might include altering the assumed relation size, > * removing an index, or adding a hypothetical index to the indexlist. > */ > if (get_relation_info_hook) > (*get_relation_info_hook) (root, relationObjectId, inhparent, rel); > > reminds me that the design intention was that hypothetical indexes > would get added to the list by get_relation_info_hook itself. > If that's not how the index adviser is operating, maybe we need > to have a discussion about that. Historically, to avoid having to hand-create the IndexOptInfo and risk getting something wrong, the Index Adviser has used index_create() to create a full-blown btree index, (sans that actual build step, with skip_build = true), and saving the returned OID. This ensured that all the catalog entries were in place before it called the standard_planner(). This way Postgres would build IndexOptInfo from the entries in the catalog, as usual. Then, inside the get_relation_info_hook() callback, Index Adviser identifies these virtual indexes by their OID, and at that point marks them with hypothetical=true. After planning is complete, the Index Adviser scans the plan to find any IndexScan objects that have indexid matching the saved OIDs. Index Adviser performs the whole thing in a subtransaction, which gets rolled back. So the hypothetical indexes are not visible to any other transaction, ever. Assigning OID to a hypothetical index is necessary, and I believe index_create() is the right way to do it. In fact, in the 9.1 cycle there was a bug fixed (a2095f7fb5, where the hypothetical flag was also invented), to solve precisely this problem; to allow the Index Adviser to use OIDs to identify hypothetical indexes that were used/chosen by the planner. But now I believe this architecture of the Index Adviser needs to change, primarily to alleviate the performance impact of creating catalog entries, subtransaction overhead, and the catalog bloat caused by index_create() (and then rolling back the subtransaction). As part of this architecture change, the Index Adviser will have to cook up IndexOptInfo objects and append them to the relation. And that should line up with the design intention you mention. But the immediate blocker is how to assign OIDs to the hypothetical indexes so that all hypothetical indexes chosen by the planner can be identified by the Index Adviser. I'd like the Index Adviser to work on read-only /standby nodes as well, but that wouldn't be possible because calling GetNewObjectId() is not allowed during recovery. I see that HypoPG uses a neat little hack [1]. Perhaps Index Adviser will also have to resort to that trick. [1]: hypo_get_min_fake_oid() finds the usable oid range below FirstNormalObjectId https://github.com/HypoPG/hypopg/blob/57d832ce7a2937fe7d42b113c7e95dd1f129795b/hypopg.c#L458 Best regards, Gurjeet http://Gurje.et
Re: table_open/table_close with different lock mode
On Thu, Jul 20, 2023 at 11:38 PM Junwang Zhao wrote: > > On Fri, Jul 21, 2023 at 2:26 PM Michael Paquier wrote: > > > > On Fri, Jul 21, 2023 at 02:05:56PM +0800, Junwang Zhao wrote: > > > I noticed there are some places calling table_open with > > > RowExclusiveLock but table_close with NoLock, like in function > > > toast_save_datum. > > > > > > Can anybody explain the underlying logic, thanks in advance. > > > > This rings a bell. This is a wanted behavior, see commit f99870d and > > its related thread: > > https://postgr.es/m/17268-d2fb426e0895a...@postgresql.org > > > > I see this patch, so all the locks held by a transaction will be released > at the commit phase, right? Can you show me where the logic is located? The NoLock is simple a marker that tells the underlying machinery to not bother releasing any locks. As a matter of fact, you can pass NoLock in *_open() calls, too, to indicate that you don't want any new locks, perhaps because the transaction has already taken an appropriate lock on the object. As for lock-releasing codepath at transaction end, see CommitTransaction() in xact.c, and specifically at the ResourceOwnerRelease() calls in there. Best regards, Gurjeet http://Gurje.et
Re: MERGE ... RETURNING
On Mon, Jul 17, 2023 at 12:43 PM Jeff Davis wrote: > > On Fri, 2023-07-14 at 09:55 +0100, Dean Rasheed wrote: > > I found a 10-year-old thread discussing adding support for OLD/NEW to > > RETURNING [1], but it doesn't look like anything close to a > > committable solution was developed, or even a design that might lead > > to one. That's a shame, because there seemed to be a lot of demand > > for > > the feature, but it's not clear how much effort it would be to > > implement. > > It looks like progress was made in the direction of using a table alias > with executor support to bring the right attributes along. That patch introduced RTE_ALIAS to carry that info through to the executor, and having to special-case that that in many places was seen as a bad thing. > There was some concern about how exactly the table alias should work > such that it doesn't look too much like a join. Not sure how much of a > problem that is. My understanding of that thread is that the join example Robert shared was for illustrative purposes only, to show that executor already has enough information to produce the desired output, and to show that it's a matter of tweaking the parser and planner to tell the executor what output to produce. But later reviewers pointed out that it's not that simple (example was given of ExecDelete performing pullups/working hard to get the correct values of the old version of the row). The concerns were mainly around use of OLD.* and NEW.*, too much special-casing of RTE_ALIAS, and then the code quality of the patch itself. > > > Maybe instead of a function it could be a special table reference > > > like: > > > > > > MERGE ... RETURNING MERGE.action, MERGE.action_number, id, val? > > > > > > > Well, that's a little more concise, but I'm not sure that it really > > buys us that much, to be worth the extra complication. Presumably > > something in the planner would turn that into something the executor > > could handle, which might just end up being the existing functions > > anyway. > > The benefits are: > > 1. It is naturally constrained to the right context. +1 > I'm not sure how much extra complication it would cause, though. If that attempt with UPDATE RETURNING a decade ago is any indication, it's probably a tough one. Best regards, Gurjeet http://Gurje.et
Re: MERGE ... RETURNING
On Fri, Jul 14, 2023 at 1:55 AM Dean Rasheed wrote: > > On Thu, 13 Jul 2023 at 20:14, Jeff Davis wrote: > > > > On Thu, 2023-07-13 at 18:01 +0100, Dean Rasheed wrote: > > > For some use cases, I can imagine allowing OLD/NEW.colname would mean > > > you wouldn't need pg_merge_action() (if the column was NOT NULL), so > > > I > > > think the features should work well together. > > > > For use cases where a user could do it either way, which would you > > expect to be the "typical" way (assuming we supported the new/old)? > > > > MERGE ... RETURNING pg_merge_action(), id, val; > > > > or > > > > MERGE ... RETURNING id, OLD.val, NEW.val; > > > > ? > > > > I think it might depend on whether OLD.val and NEW.val were actually > required, but I think I would still probably use pg_merge_action() to > get the action, since it doesn't rely on specific table columns being > NOT NULL. +1. It would be better to expose the action explicitly, rather than asking the user to deduce it based on the old and new values of a column. The server providing that value is better than letting users rely on error-prone methods. > I found a 10-year-old thread discussing adding support for OLD/NEW to > RETURNING [1], Thanks for digging up that thread. An important concern brought up in that thread was how the use of names OLD and NEW will affect plpgsql (an possibly other PLs) trigger functions, which rely on specific meaning for those names. The names BEFORE and AFTER, proposed there are not as intuitive as OLD/NEW for the purpose of identifying old and new versions of the row, but I don't have a better proposal. Perhaps PREVIOUS and CURRENT? > but it doesn't look like anything close to a > committable solution was developed, or even a design that might lead > to one. That's a shame, because there seemed to be a lot of demand for > the feature, +1 > > I am still bothered that pg_merge_action() is so context-sensitive. > > "SELECT pg_merge_action()" by itself doesn't make any sense, but it's > > allowed in the v8 patch. We could make that a runtime error, which > > would be better, but it feels like it's structurally wrong. This is not > > an objection, but it's just making me think harder about alternatives. > > > > Maybe instead of a function it could be a special table reference like: > > > > MERGE ... RETURNING MERGE.action, MERGE.action_number, id, val? I believe Jeff meant s/action_number/when_number/. Not that we've settled on a name for this virtual column. > Well, that's a little more concise, but I'm not sure that it really > buys us that much, to be worth the extra complication. After considering the options, and their pros and cons (ease of implementation, possibility of conflict with SQL spec, intuitiveness of syntax), I'm now strongly leaning towards the SQL syntax variant. Exposing the action taken via a context-sensitive function feels kludgy, when compared to Jeff's proposed SQL syntax. Don't get me wrong, I still feel it was very clever how you were able to make the function context sensitive, and make it work in expressions deeper in the subqueries. Plus, if we were able to make it work as SQL syntax, it's very likely we can use the same technique to implement BEFORE and AFTER behaviour in UPDATE ... RETURNING that the old thread could not accomplish a decade ago. > Presumably > something in the planner would turn that into something the executor > could handle, which might just end up being the existing functions > anyway. If the current patch's functions can serve the needs of the SQL syntax variant, that'd be a neat win! Best regards, Gurjeet http://Gurje.et
Re: There should be a way to use the force flag when restoring databases
On Thu, Jul 20, 2023 at 2:10 AM Daniel Gustafsson wrote: > > > On 19 Jul 2023, at 19:28, Gurjeet Singh wrote: > > > > The docs for 'pg_restore --create` say "Create the database before > > restoring into it. If --clean is also specified, drop and recreate the > > target database before connecting to it." > > > > If we provided a force option, it may then additionally say: "If the > > --clean and --force options are specified, DROP DATABASE ... WITH > > FORCE command will be used to drop the database." > > pg_restore --clean refers to dropping any pre-existing database objects and > not > just databases, but --force would only apply to databases. > > I wonder if it's worth complicating pg_restore with that when running dropdb > --force before pg_restore is an option for those wanting to use WITH FORCE. Fair point. But the same argument could've been applied to --clean option, as well; why overload the meaning of --clean and make it drop database, when a dropdb before pg_restore was an option. IMHO, if pg_restore offers to drop database, providing an option to the user to do it forcibly is not that much of a stretch, and within reason for the user to expect it to be there, like Joan did. Best regards, Gurjeet http://Gurje.et
Re: Issue in _bt_getrootheight
On Tue, Jul 11, 2023 at 9:35 AM Ahmed Ibrahim
wrote:
>
> We have been working on the pg_adviser extension whose goal is to suggest
> indexes by creating virtual/hypothetical indexes and see how it affects the
> query cost.
>
> The hypothetical index shouldn't take any space on the disk (allocates 0
> pages) so we give it the flag INDEX_CREATE_SKIP_BUILD.
> But the problem comes from here when the function get_relation_info is called
> in planning stage, it tries to calculate the B-Tree height by calling
> function _bt_getrootheight, but the B-Tree is not built at all, and its
> metadata page (which is block 0 in our case) doesn't exist, so this returns
> error that it cannot read the page (since it doesn't exist).
>
> I tried to debug the code and found that this feature was introduced in
> version 9.3 under this commit [1]. I think that in the code we need to check
> if it's a B-Tree index AND the index is built/have some pages, then we can go
> and calculate it otherwise just put it to -1
> I mean instead of this
> if (info->relam == BTREE_AM_OID)
> {
> /* For btrees, get tree height while we have the index open */
> info->tree_height = _bt_getrootheight(indexRelation);
> }
> else
> {
> /* For other index types, just set it to "unknown" for now */
> info->tree_height = -1;
> }
>
> The first line should be
> if (info->relam == BTREE_AM_OID && info->pages > 0)
> or use the storage manager (smgr) to know if the first block exists.
I think the better method would be to calculate the index height
*after* get_relation_info_hook is called. That way, instead of the
server guessing whether or not an index is hypothetical it can rely on
the index adviser's notion of which index is hypothetical. The hook
implementer has the opportunity to not only mark the
indexOptInfo->hypothetical = true, but also calculate the tree height,
if they can.
Please see attached the patch that does this. Let me know if this patch helps.
Best regards,
Gurjeet
http://Gurje.et
calculate-index-height-after-calling-get_relation_info_hook.patch
Description: Binary data
Re: There should be a way to use the force flag when restoring databases
On Tue, Jul 18, 2023 at 12:53 AM Joan wrote: > > Since posgres 13 there's the option to do a FORCE when dropping a database > (so it disconnects current users) Documentation here: > https://www.postgresql.org/docs/current/sql-dropdatabase.html > > I am currently using dir format for the output >pg_dump -d "bdname" -F d -j 4 -v -f /tmp/dir > > And restoring the database with > pg_restore -d postgres -C -c --exit-on-error -F d -j 3 -v /tmp/dir > > Having an option to add the FORCE option to either the generated dump by > pg_dump, or in the pg_restore would be very useful when restoring the > databases to another servers so it would avoid having to do scripting. > > In my specific case I am using this to refresh periodically a development > environment with data from production servers for a small database (~200M). Making force-drop a part of pg_dump output may be dangerous, and not provide much flexibility at restore time. Adding a force option to pg_restore feels like providing the right tradeoff. The docs for 'pg_restore --create` say "Create the database before restoring into it. If --clean is also specified, drop and recreate the target database before connecting to it." If we provided a force option, it may then additionally say: "If the --clean and --force options are specified, DROP DATABASE ... WITH FORCE command will be used to drop the database." Using WITH FORCE is not a guarantee, as the DROP DATABASE docs clarify the conditions under which it may fail. Best regards, Gurjeet http://Gurje.et
Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb
On Mon, Jul 17, 2023 at 1:47 PM Nathan Bossart wrote: > > Here is a new version of the patch in which I've updated this comment as > proposed. Gurjeet, do you have any other concerns about this patch? With the updated comment, the patch looks good to me. Best regards, Gurjeet http://Gurje.et
Re: Fix search_path for all maintenance commands
On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston wrote: > > I'm against simply breaking the past without any recourse as what we did for > pg_dump/pg_restore still bothers me. I'm sure this is tangential, but can you please provide some context/links to the change you're referring to here. Best regards, Gurjeet http://Gurje.et
Re: Fix search_path for all maintenance commands
On Thu, Jul 13, 2023 at 11:56 AM Jeff Davis wrote:
>
> The current approach seemed to get support from Noah, Nathan, and Greg
> Stark.
>
> Concerns were raised by Gurjeet, Tom, and Robert in the 16 cycle; but
I didn't see Tom's or Robert's concerns raised in this thread. I see
now that for some reason there are two threads with slightly different
subjects. I'll catch-up on that, as well.
The other thread's subject: "pgsql: Fix search_path to a safe value
during maintenance operations"
> I'm not sure whether those objections were specific to the 16 cycle or
> whether they are objections to the approach entirely. Comments?
The approach seems good to me. My concern is with this change's
potential to cause an extended database outage. Hence sending it out
as part of v16, without any escape hatch for the DBA, is my objection.
It it were some commercial database, we would have simply introduced a
hidden event, or a GUC, with default off. But a GUC for this feels too
heavy handed.
Perhaps we can provide an escape hatch as follows (warning, pseudocode ahead).
> if (first_element(search_path) !=
> "dont_override_search_patch_for_maintenance")
> SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, ...);
So, if someone desperately wants to just plow through the maintenance
command, and are not ready or able to fix their dependence on their
search_path, the community can show them this escape-hatch.
Best regards,
Gurjeet
http://Gurje.et
Re: MERGE ... RETURNING
On Thu, Jul 13, 2023 at 8:38 AM Dean Rasheed wrote:
>
> On Tue, 11 Jul 2023 at 21:43, Gurjeet Singh wrote:
> > { oid => '9499', descr => 'command type of current MERGE action',
> > - proname => 'pg_merge_action', provolatile => 'v',
> > + proname => 'pg_merge_action', provolatile => 'v', proparallel => 'r',
> >
> > { oid => '9500', descr => 'index of current MERGE WHEN clause',
> > - proname => 'pg_merge_when_clause', provolatile => 'v',
> > + proname => 'pg_merge_when_clause', provolatile => 'v', proparallel =>
> > 'r',
> >
> >
> > I see that you've now set proparallel of these functions to 'r'. I'd
> > just like to understand how you got to that conclusion.
> >
>
> Now that these functions can appear in subqueries in the RETURNING
> list, there exists the theoretical possibility that the subquery might
> use a parallel plan (actually that can't happen today, for any query
> that modifies data, but maybe someday it may become a possibility),
> and it's possible to use these functions in a SELECT query inside a
> PL/pgSQL function called from the RETURNING list, which might consider
> a parallel plan. Since these functions rely on access to executor
> state that isn't copied to parallel workers, they must be run on the
> leader, hence I think PARALLEL RESTRICTED is the right level to use. A
> similar example is pg_trigger_depth().
Thanks for the explanation. That helps.
Best regards,
Gurjeet
http://Gurje.et
Re: Duplicated LLVMJitHandle->lljit assignment?
On Wed, Jul 12, 2023 at 5:22 PM Matheus Alcantara wrote: > > I was reading the jit implementation and I notice that the lljit field of > LLVMJitHandle is being assigned twice on llvm_compile_module function, is this > correct? I'm attaching a supposed fixes that removes the second assignment. I > ran meson test and all tests have pass. -handle->lljit = compile_orc; LGTM. Best regards, Gurjeet http://Gurje.et
Re: CommandStatus from insert returning when using a portal.
On Wed, Jul 12, 2023 at 1:03 PM Dave Cramer wrote: > > With a simple insert such as > > INSERT INTO test_table (cnt) VALUES (1), (2) RETURNING id > > if a portal is used to get the results then the CommandStatus is not returned > on the execute only when the portal is closed. After looking at this more it > is really after all of the data is read which is consistent if you don't use > a portal, however it would be much more useful if we received the > CommandStatus after the insert was completed and before the data > > Obviously I am biased by the JDBC API which would like to have > > PreparedStatement.execute() return the number of rows inserted without having > to wait to read all of the rows returned I believe if RETURNING clause is use, the protocol-level behaviour of INSERT is expected to match that of SELECT. If the SELECT command behaves like that (resultset followed by CommandStatus), then I'd say the INSERT RETURNING is behaving as expected. Best regards, Gurjeet http://Gurje.et
Re: Better help output for pgbench -I init_steps
On Wed, Jul 12, 2023 at 3:08 AM Peter Eisentraut wrote: > > On 12.07.23 09:42, Gurjeet Singh wrote: > > These two variants show the two extremes; bare minimum vs. everything > > but the kitchen sink. So one may feel the need to find a middle ground > > and provide a "sufficient, but not too much" variant. I have attempted > > that in variants 3 and 4; also attached. > > If you end up with variant 3 or 4, please use double quotes instead of > single quotes. Will do. I believe you're suggesting this because in the neighboring help text the string literals use double quotes. I see that other utilities, such as psql also use double quotes in help text. If there's a convention, documented somewhere in our sources, I'd love to know and learn other conventions. Best regards, Gurjeet http://Gurje.et
Better help output for pgbench -I init_steps
These patches were created during an unrelated discussion about pgbench. Please see emails [1] - [6] linked below, for the past discussion. In brief: > $ pgbench -i -I dtGvp -s 500 The init-steps are severely under-documented in pgbench --help output. I think at least a pointer to the the pgbench docs should be mentioned in the pgbench --help output; an average user may not rush to read the code to find the explanation, but a hint to where to find more details about what the letters in --init-steps mean, would save them a lot of time. Please see attached 4 variants of the patch. Variant 1 simply tells the reader to consult pgbench documentation. The second variant provides a description for each of the letters, as the documentation does. The committer can pick the one they find suitable. The text ", in the specified order" is an important detail, that should be included irrespective of the rest of the patch. My preference would be to use the first variant, since the second one feels too wordy for --help output. I believe we'll have to choose between these two; the alternatives will not make anyone happy. These two variants show the two extremes; bare minimum vs. everything but the kitchen sink. So one may feel the need to find a middle ground and provide a "sufficient, but not too much" variant. I have attempted that in variants 3 and 4; also attached. The third variant does away with the list of steps, and uses a paragraph to describe the letters. And the fourth variant makes that paragraph terse. In the order of preference I'd choose variant 1, then 2. Variants 3 and 4 feel like a significant degradation from variant 2. Attached samples.txt shows the snippets of --help output of each of the variants/patches, for comparison. In [6] below, Tristan showed preference for the second variant. [1] My complaint about -I initi_steps being severely under-documented in --help output https://www.postgresql.org/message-id/CABwTF4XMdHTxemhskad41Vj_hp2nPgifjwegOqR52_8-wEbv2Q%40mail.gmail.com [2] Tristan Partin agreeing with the complaint, and suggesting a patch would be welcome https://www.postgresql.org/message-id/CT8BC7RXT33R.3CHYIXGD5NVHK%40gonk [3] Greg Smith agreeing and saying he'd welcome a few more words about the init_steps in --help output https://www.postgresql.org/message-id/CAHLJuCUp5_VUo%2BRJ%2BpSnxeiiZfcstRtTubRP8%2Bu8NEqmrbp4aw%40mail.gmail.com [4] First set of patches https://www.postgresql.org/message-id/CABwTF4UKv43ZftJadsxs8%3Da07BmA1U4RU3W1qbmDAguVKJAmZw%40mail.gmail.com [5] Second set of patches https://www.postgresql.org/message-id/CABwTF4Ww42arY1Q88_iaraVLxqSU%2B8Yb4oKiTT5gD1sineog9w%40mail.gmail.com [6] Tristan showing preference for the second variant https://www.postgresql.org/message-id/CTBN5E2K2YSJ.3QYXGZ09JZXIW%40gonk +CC Tristan Partin and Greg Smith, since they were involved in the initial thread. Best regards, Gurjeet http://Gurje.et variant-1-brief-pointer-to-docs.patch -i, --initialize invokes initialization mode -I, --init-steps=[dtgGvpf]+ (default "dtgvp") run selected initialization steps, in the specified order see pgbench documentation for a description of these steps -F, --fillfactor=NUM set fill factor variant-2-detailed-description.patch -i, --initialize invokes initialization mode -I, --init-steps=[dtgGvpf]+ (default "dtgvp") run selected initialization steps, in the specified order d: Drop any existing pgbench tables t: Create the tables used by the standard pgbench scenario g: Generate data, client-side G: Generate data, server-side v: Invoke VACUUM on the standard tables p: Create primary key indexes on the standard tables f: Create foreign key constraints between the standard tables -F, --fillfactor=NUM set fill factor variant-3-details-not-list.patch -i, --initialize invokes initialization mode -I, --init-steps=[dtgGvpf]+ (default "dtgvp") run selected initialization steps, in the specified order. These steps operate on standard tables used by pgbench 'd' drops the tables, 't' creates the tables, 'g' generates the data on client-side, 'G' generates data on server-side, 'v' vaccums the tables, 'p' creates primary key constraints, and 'f' creates foreign key constraints -F, --fillfactor=NUM set fill factor variant-4-terse-details-not-list.patch -i, --initialize invokes initialization mode -I, --init-steps=[dtgGvpf]+ (default "dtgvp") run selected initialization steps, in the specified order.
Re: MERGE ... RETURNING
On Tue, Jul 11, 2023 at 1:43 PM Gurjeet Singh wrote: > > In the above hunk, if there's an exception/ERROR, I believe we should > PG_RE_THROW(). If there's a reason to continue, we should at least set > rslot = NULL, otherwise we may be returning an uninitialized value to > the caller. Excuse the brain-fart on my part. There's not need to PG_RE_THROW(), since there's no PG_CATCH(). Re-learning the code's infrastructure slowly :-) Best regards, Gurjeet http://Gurje.et
Re: MERGE ... RETURNING
On Fri, Jul 7, 2023 at 3:48 PM Dean Rasheed wrote:
>
> On Thu, 6 Jul 2023 at 06:13, Gurjeet Singh wrote:
> >
> > > One minor annoyance is that, due to the way that pg_merge_action() and
> > > pg_merge_when_clause() require access to the MergeActionState, they
> > > only work if they appear directly in the RETURNING list. They can't,
> > > for example, appear in a subquery in the RETURNING list, and I don't
> > > see an easy way round that limitation.
> >
> > I believe that's a serious limitation, and would be a blocker for the
> > feature.
>
> Yes, that was bugging me for quite a while.
>
> Attached is a new version that removes that restriction, allowing the
> merge support functions to appear anywhere. This requires a bit of
> planner support, to deal with merge support functions in subqueries,
> in a similar way to how aggregates and GROUPING() expressions are
> handled. But a number of the changes from the previous patch are no
> longer necessary, so overall, this version of the patch is slightly
> smaller.
+1
> > I think the name of function pg_merge_when_clause() can be improved.
> > How about pg_merge_when_clause_ordinal().
> >
>
> That's a bit of a mouthful, but I don't have a better idea right now.
> I've left the names alone for now, in case something better occurs to
> anyone.
+1. How do we make sure we don't forget that it needs to be named
better. Perhaps a TODO item within the patch will help.
> > In the doc page of MERGE, you've moved the 'with_query' from the
> > bottom of Parameters section to the top of that section. Any reason
> > for this? Since the Parameters section is quite large, for a moment I
> > thought the patch was _adding_ the description of 'with_query'.
> >
>
> Ah yes, I was just making the order consistent with the
> INSERT/UPDATE/DELETE pages. That could probably be committed
> separately.
I don't think that's necessary, if it improves consistency with related docs.
> > +/*
> > + * Merge support functions should only be called directly from a MERGE
> > + * command, and need access to the parent ModifyTableState. The parser
> > + * should have checked that such functions only appear in the RETURNING
> > + * list of a MERGE, so this should never fail.
> > + */
> > +if (IsMergeSupportFunction(funcid))
> > +{
> > +if (!state->parent ||
> > +!IsA(state->parent, ModifyTableState) ||
> > +((ModifyTableState *) state->parent)->operation != CMD_MERGE)
> > +elog(ERROR, "merge support function called in non-merge
> > context");
> >
> > As the comment says, this is an unexpected condition, and should have
> > been caught and reported by the parser. So I think it'd be better to
> > use an Assert() here. Moreover, there's ERROR being raised by the
> > pg_merge_action() and pg_merge_when_clause() functions, when they
> > detect the function context is not appropriate.
> >
> > I found it very innovative to allow these functions to be called only
> > in a certain part of certain SQL command. I don't think there's a
> > precedence for this in Postgres; I'd be glad to learn if there are
> > other functions that Postgres exposes this way.
> >
> > -EXPR_KIND_RETURNING,/* RETURNING */
> > +EXPR_KIND_RETURNING,/* RETURNING in INSERT/UPDATE/DELETE */
> > +EXPR_KIND_MERGE_RETURNING, /* RETURNING in MERGE */
> >
> > Having to invent a whole new ParseExprKind enum, feels like a bit of
> > an overkill. My only objection is that this is exactly like
> > EXPR_KIND_RETURNING, hence EXPR_KIND_MERGE_RETURNING needs to be dealt
> > with exactly in as many places. But I don't have any alternative
> > proposals.
> >
>
> That's all gone now from the new patch, since there is no longer any
> restriction on where these functions can appear.
I believe this elog can be safely turned into an Assert.
+switch (mergeActionCmdType)
{
-CmdType commandType = relaction->mas_action->commandType;
+case CMD_INSERT:
+default:
+elog(ERROR, "unrecognized commandType: %d", (int)
mergeActionCmdType);
The same treatment can be applied to the elog(ERROR) in pg_merge_action().
> > +CREATE FUNCTION merge_into_sq_target(sid int, balance int, delta int,
> > + OUT action text, OUT tid int,
> > OUT new_balance int)
> > +LANGUAGE sql AS
> > +$$
> > +MERGE INTO sq_target t
> > +USING (VALUES ($1, $2, $3)) AS v(sid, balance, delta)
> > +ON t
Re: Document that server will start even if it's unable to open some TCP/IP ports
On Mon, Jun 19, 2023 at 5:48 PM Bruce Momjian wrote: > > On Tue, Jun 13, 2023 at 11:11:04PM -0400, Tom Lane wrote: > > > > There is certainly an argument that such a condition indicates that > > something's very broken in our configuration and we should complain. > > But I'm not sure how exciting the case is in practice. The systemd > > guys would really like us to be willing to come up before any network > > interfaces are up, and then auto-listen to those interfaces when they > > do come up. That sounds like a reasonable expectation, as the network conditions can change without any explicit changes made by someone. > > On the other hand, the situation with Unix sockets is > > much more static: if you can't make a socket in /tmp or /var/run at > > the instant of postmaster start, it's unlikely you will be able to do > > so later. I think you're describing a setup where Postgres startup is automatic, as part of server/OS startup. That is the most common case. In cases where someone is performing a Postgres startup manually, they are very likely to have the permissions to fix the problem preventing the startup. > > Maybe we need different rules for TCP versus Unix-domain sockets? > > I'm not sure what exactly, but lumping those cases together for > > a discussion like this feels wrong. +1. > If we are going to retry for network configuration changes, it seems we > would also retry Unix domain sockets for cases like when the permissions > are wrong, and then fixed. The network managers (systemd, etc.) are expected to respond to dynamic conditions, and hence they may perform network config changes in response to things like network outages, and hardware failures, time of day, etc. On the other hand, the permissions required to create files for Unix domain sockets are only expected to change if someone decides to make that change. I wouldn't expect these permissions to be changed dynamically. On those grounds, keeping the treatment of Unix domain sockets out of this discussion for this patch seems reasonable. > However, it seem hard to figure out exactly what _is_ working if we take > the approach of dynamically retrying listen methods. Do we report > anything helpful in the server logs when we start and can't listen on > anything? Yes. For every host listed in listen_addresses, if Postgres fails to open the port on that address, we get a WARNING message in the server log. After the end of processing of a non-empty listen_addresses, if there are zero open TCP/IP connections, the server exits (with a FATAL message, IIRC). Best regards, Gurjeet http://Gurje.et
Re: BUG #18016: REINDEX TABLE failure
On Sun, Jul 9, 2023 at 7:18 AM Tom Lane wrote: > > Michael Paquier writes: > > That should be OK, I assume. However, if this is improved and > > something we want to support in the long-run I guess that a TAP test > > may be appropriate. > > I do not see the point of a TAP test. It's not like the code isn't > covered perfectly well. Please find attached the patch that makes REINDEX TABLE perform reindex on toast table before reindexing the main table's indexes. The code block movement involved slightly more thought and care than I had previously imagined. As explained in comments in the patch, the enumeration and suppression of indexes on the main table must happen before any CommandCounterIncrement() call, hence the reindex-the-toast-table-if-any code had to be placed after that enumeration. In support of the argument above, the patch does not include any TAP tests. Reliably reproducing the original error message involves restarting the database, and since that can't be done via SQL commands, no sql tests are included, either. The patch also includes minor wordsmithing, and benign whitespace changes in neighboring code. Best regards, Gurjeet http://Gurje.et v1-0001-Reindex-toast-table-s-index-before-main-table-s-i.patch Description: Binary data
Re: BUG #18016: REINDEX TABLE failure
On Sun, Jul 9, 2023 at 7:21 AM Richard Veselý wrote: > > ... there's no shortage of people that suffer from sluggish > pg_dump/pg_restore cycle and I imagine there are any number of people that > would be interested in improving bulk ingestion which is often a bottleneck > for analytical workloads as you are well aware. What's the best place to > discuss this topic further - pgsql-performance or someplace else? (moved conversation to -hackers, and moved -bugs to BCC) > I was dissatisfied with storage layer performance, especially during the > initial database population, so I rewrote it for my use case. I'm done with > the heap, but for the moment I still rely on PostgreSQL to build indexes, It sounds like you've developed a method to speed up loading of tables, and might have ideas/suggestions for speeding up CREATE INDEX/REINDEX. The -hackers list feels like a place to discuss such changes. Best regards, Gurjeet http://Gurje.et
Re: DecodeInterval fixes
On Sat, Jul 8, 2023 at 1:33 PM Tom Lane wrote: > > Gurjeet Singh writes: > > On Fri, Jul 7, 2023 at 4:13 PM Tom Lane wrote: > >> The ECPG datetime datatype support code has been basically unmaintained > >> for years, and has diverged quite far at this point from the core code. > > > I was under the impression that anything in the postgresql.git > > repository is considered core, and hence maintained as one unit, from > > release to release. > > When I say that ecpglib is next door to unmaintained, I'm just stating > the facts on the ground; project policy is irrelevant. That situation > is not likely to change until somebody steps up to do (a lot of) work > on it, which is probably unlikely to happen unless we start getting > actual complaints from ECPG users. For the meantime, what's there now > seems to be satisfactory to whoever is using it ... which might be > nobody? > > In any case, you don't have to look far to notice that some parts of > the tree are maintained far more actively than others. ecpglib is > just one of the more identifiable bits that's receiving little love. > The quality of the code under contrib/ is wildly variable, and even > the server code itself has backwaters. (For instance, the "bit" types, > which aren't even in the standard anymore; or the geometric types, > or "money".) Thanks for sharing your view on different parts of the code. This give a clear direction if someone would be interested in stepping up. As part of my mentoring a GSoC 2023 participant, last night we were looking at the TODO wiki page, for something for the mentee to pick up next. I feel the staleness/deficiencies you mention above are not captured in the TODO wiki page. It'd be nice if these were documented, so that newcomers to the community can pick up work that they feel is an easy lift for them. > By and large, I don't see this unevenness of maintenance effort as > a problem. It's more like directing our limited resources into the > most useful places. Code that isn't getting worked on is either not > used at all by anybody, or it serves the needs of those who use it > well enough already. Since it's difficult to tell which of those > cases applies, removing code just because it's not been improved > lately is a hard choice to sell. But so is putting maintenance effort > into code that there might be little audience for. In the end we > solve this via the principle of "scratch your own itch": if somebody > is concerned enough about a particular piece of code to put their own > time into improving it, then great, it'll get improved. > > > Benign neglect doesn't sound nice from a user's/consumer's > > perspective. Can it be labeled (i.e. declared as such in docs) as > > deprecated. > > Deprecation would imply that we're planning to remove it, which > we are not. Good to know. Sorry I took "benign neglect" to mean that there's no willingness to improve it. This makes it clear that community wants to improve and maintain ECPG, it's just a matter of someone volunteering, and better use of the resources available. Best regards, Gurjeet http://Gurje.et
Re: DecodeInterval fixes
On Fri, Jul 7, 2023 at 4:13 PM Tom Lane wrote: > > The ECPG datetime datatype support code has been basically unmaintained > for years, and has diverged quite far at this point from the core code. I was under the impression that anything in the postgresql.git repository is considered core, and hence maintained as one unit, from release to release. An example of this, to me, were all the contrib/* modules. > I wouldn't expect that a patch to the core code necessarily applies > easily to ECPG, nor would I expect somebody patching the core to bother > trying. The above statement makes me think that only the code inside src/backend/ is considered core. Is that the right assumption? > Perhaps modernizing/resyncing that ECPG code would be a worthwhile > undertaking, but it'd be a mighty large one, and I'm not sure about > the size of the return. In the meantime, benign neglect is the policy. Benign neglect doesn't sound nice from a user's/consumer's perspective. Can it be labeled (i.e. declared as such in docs) as deprecated. Knowing that the tool you use has now been deprecated would be a better message for someone still using it, even if it was left marked deprecated indefinitely. Discovering benign neglect for the tool you depend on, from secondary sources (like this list, forums, etc.), does not evoke a lot of confidence. Best regards, Gurjeet http://Gurje.et
Re: Standardize type of variable when extending Buffers
On Fri, Jul 7, 2023 at 6:12 AM Ranier Vilela wrote: > > Hi, > > This has already been discussed in [1]. > But I thought it best to start a new thread. > > The commit 31966b1 introduced the infrastructure to extend > buffers. > But the patch mixed types with int and uint32. > The correct type of the variable counter is uint32. > > Fix by standardizing the int type to uint32. > > patch attached. LGTM. +CC Kyotaro, as they were involved in the previous discussion. > > [1] > https://www.postgresql.org/message-id/CAEudQAr_oWHpZk4uumZijYS362gp4KHAah-yUe08CQY4a4SsOQ%40mail.gmail.com Best regards, Gurjeet http://Gurje.et
Re: [17] CREATE COLLATION default provider
On Fri, Jul 7, 2023 at 9:33 AM Jeff Davis wrote: > > On Sat, 2023-06-17 at 09:09 -0700, Gurjeet Singh wrote: > > The docs for the CREATE COLLATION option 'locale' say: "This is a > > shortcut for setting LC_COLLATE and LC_CTYPE at once." > > > > So it's not intuitive why the check does not include a test for the > > presence of 'localeEl', as well? If we consider the presence of > > LC_COLLATE _or_ LC_CTYPE options to be a determining factor for some > > decision, then the presence of LOCALE option should also lead to the > > same outcome. > > > > The docs say: "If provider is libc, this is a shortcut...". The point > is that LC_COLLATE and LC_CTYPE act as a signal that what the user > really wants is a libc collation. LOCALE works for either, so we need a > default. Sorry about the noise, I was consulting current/v15 docs online. Now that v16 docs are online, I can see that the option in fact says this is the case only if libc is the provider. (note to self: for reviewing patches to master, consult devel docs [1] online) [1]: https://www.postgresql.org/docs/devel/ Best regards, Gurjeet http://Gurje.et
Re: RFC: pg_stat_logmsg
On Thu, Jul 6, 2023 at 12:37 AM Masahiko Sawada wrote:
> On Sat, Jul 1, 2023 at 8:57 AM Joe Conway wrote:
> >
> > The basic idea is to mirror how pg_stat_statements works, except the
> > logged messages keyed by filename, lineno, and elevel are saved with a
> > aggregate count. The format string is displayed (similar to a query
> > jumble) for context, along with function name and sqlerrcode.
> >
> >
> > Part of the thinking is that people with fleets of postgres instances
> > can use this to scan for various errors that they care about.
> > Additionally it would be useful to look for "should not happen" errors.
> I'm concerned that displaying the format string could not be helpful
> in some cases. For example, when raising an ERROR while reading WAL
> records, we typically write the error message stored in
> record->errormsg_buf:
>
> in ReadRecord():
> if (errormsg)
> ereport(emode_for_corrupt_record(emode,
> xlogreader->EndRecPtr),
> (errmsg_internal("%s", errormsg) /* already
> translated */ ));
>
> In this case, the error message stored in pg_stat_logmsg is just '%s'.
> The filename and line number columns might help identify the actual
> error but it requires users to read the source code and cannot know
> the actual error message.
I believe that the number of such error messages, the ones with very
little descriptive content, is very low in Postgres code. These kinds
of messages are not prevalent, and must be used when code hits obscure
conditions, like seen in your example above. These are the kinds of
errors that Joe is referring to as "should not happen". In these
cases, even if the error message was descriptive, the user will very
likely have to dive deep into code to find out the real cause.
I feel that the unique combination of file name and line number is
useful information, even in cases where the format string not very
descriptive. So I believe the extension's behaviour in this regard is
acceptable.
In cases where the extension's output is not descriptive enough, the
user can use the filename:lineno pair to look for fully formed error
messages in the actual log files; they may have to make appropriate
changes to log_* parameters, though.
If we still feel strongly about getting the actual message for these
cases, perhaps we can develop a heuristic to identify such messages,
and use either full or a prefix of the 'message' field, instead of
'message_id' field. The heuristic could be: strlen(message_id) is too
short, or that message_id is all/overwhelmingly format specifiers
(e.g. '%s: %d').
The core benefit of this extension is to make it easy for the user to
discover error messages affecting their workload. The user may be
interested in knowing the most common log messages in their server
logs, so that they can work on reducing those errors or warnings. Or
they may be interested in knowing when their workload hits
unexpected/serious error messages, even if they're infrequent. The
data exposed by pg_stat_logmsg view would serve as a starting point,
but I'm guessing it would not be sufficient for their investigation.
On Fri, Jun 30, 2023 at 4:57 PM Joe Conway wrote:
> I would love to get
> feedback on:
>
> 1/ the general concept
> 2/ the pg_stat_statement-like implementation
> 3/ contrib vs core vs external project
+1 for the idea; a monitoring system trained at this view can bubble
up problems to users that may otherwise go unnoticed. Piggybacking on,
or using pg_stat_statments implementation as a model seems fine. I
believe making this available as a contrib module hits the right
balance; not all installations may want this, hence in-core, always
collecting data, seems undesirable; if it's an external project, it
won't be discoverable, I see that as a very high bar for someone to
trust it and begin to use it.
Best regards,
Gurjeet
http://Gurje.et
Re: [PATCH] Add GitLab CI to PostgreSQL
On Thu, Jul 6, 2023 at 11:27 AM Daniel Gustafsson wrote: > > > On 6 Jul 2023, at 20:10, Gurjeet Singh wrote: > > > I can > > imagine if cfbot was developed against some other CI, it's very likely > > that we'd be using that other CI instead of Cirrus. > > The CFBot originally used Travis, but switched in late 2020 when Travis almost > over night become hard to use for open source projects: > > > https://github.com/macdice/cfbot/commit/a62aa6d77dd4cc7f0a5549db378cd6f1cf25c0e2 Thanks for providing the historical context! A for-profit entity, despite their best intentions, and sometimes by no fault of their own, may not survive. It's not that a non-profits are guaranteed to survive, but the conditions they operate in are drastically different than those of for-profit ones. > These systems come and go, and each have their quirks. I'm sure the community has seen enough of such disappearances over the years, which is why I was surprised to see the adoption of Cirrus in core (after I had stopped paying attention to Postgres hackers list for a few years). Having read that whole discussion, though, I do see the immense value Cirrus CI provides. > Having options is good, > but maintaining multiple ones isn't necessarily a free fire-and-forget type of > thing for the community. By not adopting at least one other CI, it'd seem like the community is favoring Cirrus over others; and that doesn't feel good. Best regards, Gurjeet http://Gurje.et
Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c)
On Thu, Jul 6, 2023 at 8:01 AM Karina Litskevich wrote: > > >> EB_SMGR and EB_REL are macros for making new structs. >> IMO these are buggy, once make new structs without initializing all fields. >> Attached a patch to fix this and make more clear when rel or smgr is NULL. > > > As long as a structure is initialized, its fields that are not present in > initialization are initialized to zeros and NULLs depending on their types. > See C99 Standard 6.7.8.21 and 6.7.8.10. This behaviour is quite well known, > so I don't think this place is buggy. Anyway, if someone else says the code > is more readable with these fields initialized explicitly, then go on. Even though I am not a fan of the Designated Initializers feature, I agree with Karina. Per the standard, the unmentioned fields get initialized to zeroes/NULLs, so the explicit initialization to zero/null that this additional patch does is unnecessary. Moreover, I feel that it makes the code less pleasant to read. C99, 6.7.8.21: > If there are fewer initializers in a brace-enclosed list than there are > elements or members of an aggregate, or fewer characters in a string literal > used to initialize an array of known size than there are elements in the > array, > the remainder of the aggregate shall be initialized implicitly the same as > objects that have static storage duration. C99, 6.7.8.10: > If an object that has automatic storage duration is not initialized > explicitly, > its value is indeterminate. If an object that has static storage duration is > not initialized explicitly, then: > - if it has pointer type, it is initialized to a null pointer; > - if it has arithmetic type, it is initialized to (positive or unsigned) zero; > - if it is an aggregate, every member is initialized (recursively) according > to these rules; > - if it is a union, the first named member is initialized (recursively) > according to these rules. Best regards, Gurjeet http://Gurje.et
Re: [PATCH] Add GitLab CI to PostgreSQL
On Wed, Jul 5, 2023 at 6:22 AM Andrew Dunstan wrote: > > > On 2023-07-04 Tu 19:44, Newhouse, Robin wrote: > > Hello! > > > > I propose the attached patch to be applied on the 'master' branch > > of PostgreSQL to add GitLab CI automation alongside Cirrus CI in the > PostgreSQL repository. > > > > It is not intended to be a replacement for Cirrus CI, but simply suggestion > for the > > PostgreSQL project to host centrally a Gitlab CI definition for those who > prefer to use > > it while developing/testing PostgreSQL. > > > > The intent is to facilitate collaboration among GitLab users, promote > standardization > > and consistency, and ultimately, improve testing and code quality. > > > This seems very RedHat-centric, which I'm not sure is a good idea. A few years ago, a proposal to use CentOS may not have been a bad proposal. But since Redhat changed CentOS to be an upstream distro (rather than a rebuild of RHEL), I do see a reason to consider RHEL as a candidate in our CI. I think the idea of a pre-buildfarm CI is to enable contributors catch problems before they're merged, or even before proposed as a patch to the community. So if our CI includes support for a prominent Linux distro, I think it's worth it, to provide coverage for the large ecosystem that's based on RHEL and its derivatives. Would using RockyLinux assuage these concerns? Perhaps, it would. > If we're going to do this then arguably we should also be supporting GitHub > Actions and who knows what other CI frameworks. There is a case for us > special casing Cirrus CI because it's used for the cfbot. We've already lost that battle by supporting one commercial/non-community provider. From Anrdres' email [1]: > An obvious criticism of the effort to put CI runner infrastructure into core > is that they are effectively all proprietary technology, and that we should be > hesistant to depend too much on one of them. I think that's a valid > concern. However, once one CI integration is done, a good chunk (but not all!) > the work is transferrable to another CI solution, which I do think reduces the > dependency sufficiently. So it seems that supporting more than one CI was always on the cards. Cirrus was chosen for its advantages that Andres mentions in the email, but also for the reason that cfbot had chosen Cirrus. I can imagine if cfbot was developed against some other CI, it's very likely that we'd be using that other CI instead of Cirrus. [1]: https://www.postgresql.org/message-id/20211001222752.wrz7erzh4cajvgp6%40alap3.anarazel.de Best regards, Gurjeet http://Gurje.et
Re: MERGE ... RETURNING
On Thu, Jul 6, 2023 at 4:07 AM jian he wrote:
>
> On Thu, Jul 6, 2023 at 1:13 PM Gurjeet Singh wrote:
> > I think the name of function pg_merge_when_clause() can be improved.
> > How about pg_merge_when_clause_ordinal().
>
> another idea: pg_merge_action_ordinal()
Since there can be many occurrences of the same action
(INSERT/UPDATE/DELETE) in a MERGE command associated with different
conditions, I don't think action_ordinal would make sense for this
function name.
e.g.
WHEN MATCHED and src.col1 = val1 THEN UPDATE col2 = someval1
WHEN MATCHED and src.col1 = val2 THEN UPDATE col2 = someval2
...
When looking at the implementation code, as well, we see that the code
in this function tracks and reports the lexical position of the WHEN
clause, irrespective of the action associated with that WHEN clause.
foreach(l, stmt->mergeWhenClauses)
{
...
action->index = foreach_current_index(l) + 1;
Best regards,
Gurjeet
http://Gurje.et
Re: MERGE ... RETURNING
On Thu, Jul 6, 2023 at 3:39 AM Alvaro Herrera wrote: > > On 2023-Jul-05, Gurjeet Singh wrote: > > I expected the .out file to have captured the stdout. I'm gradually, > > and gladly, re-learning bits of the test infrastructure. > > > > The DELETE command tag in the output does not feel appropriate for a > > COPY command that's using MERGE as the source of the data. > > You misread this one :-) The COPY output is there, the tag is not. So > DELETE is the value from pg_merge_action(), and "1 100" correspond to > the columns in the the sq_target row that was deleted. The command tag > is presumably MERGE 1. :-) That makes more sense. It matches my old mental model. Thanks for clarifying! Best regards, Gurjeet http://Gurje.et
Re: MERGE ... RETURNING
On Sat, Jul 1, 2023 at 4:08 AM Dean Rasheed wrote:
>
> On Mon, 13 Mar 2023 at 13:36, Dean Rasheed wrote:
> >
> > And another rebase.
> >
>
> I ran out of cycles to pursue the MERGE patches in v16, but hopefully
> I can make more progress in v17.
>
> Looking at this one with fresh eyes, it looks mostly in good shape.
+1
Most of the review was done with the v6 of the patch, minus the doc
build step. The additional changes in v7 of the patch were eyeballed,
and tested with `make check`.
> To
> recap, this adds support for the RETURNING clause in MERGE, together
> with new support functions pg_merge_action() and
> pg_merge_when_clause() that can be used in the RETURNING list of MERGE
nit: s/can be used in/can be used only in/
> to retrieve the kind of action (INSERT/UPDATE/DELETE), and the index
> of the WHEN clause executed for each row merged. In addition,
> RETURNING support allows MERGE to be used as the source query in COPY
> TO and WITH queries.
>
> One minor annoyance is that, due to the way that pg_merge_action() and
> pg_merge_when_clause() require access to the MergeActionState, they
> only work if they appear directly in the RETURNING list. They can't,
> for example, appear in a subquery in the RETURNING list, and I don't
> see an easy way round that limitation.
I believe that's a serious limitation, and would be a blocker for the feature.
> Attached is an updated patch with some cosmetic updates, plus updated
> ruleutils support.
With each iteration of your patch, it is becoming increasingly clear
that this will be a documentation-heavy patch :-)
I think the name of function pg_merge_when_clause() can be improved.
How about pg_merge_when_clause_ordinal().
In the doc page of MERGE, you've moved the 'with_query' from the
bottom of Parameters section to the top of that section. Any reason
for this? Since the Parameters section is quite large, for a moment I
thought the patch was _adding_ the description of 'with_query'.
+/*
+ * Merge support functions should only be called directly from a MERGE
+ * command, and need access to the parent ModifyTableState. The parser
+ * should have checked that such functions only appear in the RETURNING
+ * list of a MERGE, so this should never fail.
+ */
+if (IsMergeSupportFunction(funcid))
+{
+if (!state->parent ||
+!IsA(state->parent, ModifyTableState) ||
+((ModifyTableState *) state->parent)->operation != CMD_MERGE)
+elog(ERROR, "merge support function called in non-merge context");
As the comment says, this is an unexpected condition, and should have
been caught and reported by the parser. So I think it'd be better to
use an Assert() here. Moreover, there's ERROR being raised by the
pg_merge_action() and pg_merge_when_clause() functions, when they
detect the function context is not appropriate.
I found it very innovative to allow these functions to be called only
in a certain part of certain SQL command. I don't think there's a
precedence for this in Postgres; I'd be glad to learn if there are
other functions that Postgres exposes this way.
-EXPR_KIND_RETURNING,/* RETURNING */
+EXPR_KIND_RETURNING,/* RETURNING in INSERT/UPDATE/DELETE */
+EXPR_KIND_MERGE_RETURNING, /* RETURNING in MERGE */
Having to invent a whole new ParseExprKind enum, feels like a bit of
an overkill. My only objection is that this is exactly like
EXPR_KIND_RETURNING, hence EXPR_KIND_MERGE_RETURNING needs to be dealt
with exactly in as many places. But I don't have any alternative
proposals.
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
+/* Is this a merge support function? (Requires fmgroids.h) */
+#define IsMergeSupportFunction(funcid) \
+((funcid) == F_PG_MERGE_ACTION || \
+ (funcid) == F_PG_MERGE_WHEN_CLAUSE)
If it doesn't cause recursion or other complications, I think we
should simply include the fmgroids.h in pg_proc.h. I understand that
not all .c files/consumers that include pg_proc.h may need fmgroids.h,
but #include'ing it will eliminate the need to keep the "Requires..."
note above, and avoid confusion, too.
--- a/src/test/regress/expected/merge.out
+++ b/src/test/regress/expected/merge.out
-WHEN MATCHED AND tid > 2 THEN
+WHEN MATCHED AND tid >= 2 THEN
This change can be treated as a bug fix :-)
+-- COPY (MERGE ... RETURNING) TO ...
+BEGIN;
+COPY (
+MERGE INTO sq_target t
+USING v
+ON tid = sid
+WHEN MATCHED AND tid > 2 THEN
For consistency, the check should be tid >= 2, like you've fixed in
command referenced above.
+BEGIN;
+COPY (
+MERGE INTO sq_target t
+USING v
+ON tid = sid
+WHEN MATCHED AND tid > 2 THEN
+UPDATE SET balance = t.balance + delta
+WHEN NOT MATCHED THEN
+INSERT (balance, tid) VALUES (balance + delta, sid)
+WHEN MATCHED AND tid < 2 THEN
+DELETE
+RETURNING pg_merge_action(), t.*
+) TO stdout;
+DELETE 1 100
+ROLLBACK;
I expected
Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb
On Tue, Jun 27, 2023 at 9:57 PM Nathan Bossart wrote:
>
> While looking at something unrelated, I noticed that the vacuumdb docs
> mention the following:
>
> vacuumdb might need to connect several times to the PostgreSQL server,
> asking for a password each time.
>
> IIUC this has been fixed since 83dec5a from 2015 (which was superceded by
> ff402ae), so I think this note (originally added in e0a77f5 from 2002) can
> now be removed.
>
> I also found that neither clusterdb nor reindexdb uses the
> allow_password_reuse parameter in connectDatabase(), and the reindexdb
> documentation contains the same note about repeatedly asking for a
> password (originally added in 85e9a5a from 2005). IMO we should allow
> password reuse for all three programs, and we should remove the
> aforementioned notes in the docs, too. This is what the attached patch
> does.
>
> Thoughts?
The comment on top of connect_utils.c:connectDatabase() seems pertinent:
> (Callers should not pass
> * allow_password_reuse=true unless reconnecting to the same database+user
> * as before, else we might create password exposure hazards.)
The callers of {cluster|reindex}_one_database() (which in turn call
connectDatabase()) clearly pass different database names in successive
calls to these functions. So the patch seems to be in conflict with
the recommendation in the comment.
I'm not sure if the concern raised in that comment is a legitimate
one, though. I mean, if the password is reused to connect to a
different database in the same cluster/instance, which I think is
always the case with these utilities, the password will exposed in the
server logs (if at all). And since the admins of the instance already
have full control over the passwords of the user, I don't think this
patch will give them any more information than what they can get
anyways.
It is a valid concern, though, if the utility connects to a different
instance in the same run/invocation, and hence exposes the password
from the first instance to the admins of the second cluster.
Nitpicking: The patch seems to have Windows line endings, which
explains why my `patch` complained so loudly.
$ patch -p1 < v1-0001-harmonize-patch
(Stripping trailing CRs from patch; use --binary to disable.)
patching file doc/src/sgml/ref/reindexdb.sgml
(Stripping trailing CRs from patch; use --binary to disable.)
patching file doc/src/sgml/ref/vacuumdb.sgml
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/bin/scripts/clusterdb.c
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/bin/scripts/reindexdb.c
$ file v1-0001-harmonize-password-reuse-in-vacuumdb-clusterdb-an.patch
v1-0001-harmonize-patch: unified diff output text, ASCII text,
with CRLF line terminators
Best regards,
Gurjeet
http://Gurje.et
Re: [17] CREATE COLLATION default provider
On Wed, Jun 14, 2023 at 9:48 PM Jeff Davis wrote: > > Currently, CREATE COLLATION always defaults the provider to libc. > > The attached patch causes it to default to libc if LC_COLLATE/LC_CTYPE > are specified, otherwise default to the current database default > collation's provider. +if (lccollateEl || lcctypeEl) +collprovider = COLLPROVIDER_LIBC; +else +collprovider = default_locale.provider; The docs for the CREATE COLLATION option 'locale' say: "This is a shortcut for setting LC_COLLATE and LC_CTYPE at once." So it's not intuitive why the check does not include a test for the presence of 'localeEl', as well? If we consider the presence of LC_COLLATE _or_ LC_CTYPE options to be a determining factor for some decision, then the presence of LOCALE option should also lead to the same outcome. Otherwise the patch looks good. > v11-0001-CREATE-COLLATION-default-provider.patch I believe v11 is a typo, and you really meant v1. Best regards, Gurjeet http://Gurje.et
Re: test_extensions: fix inconsistency between meson.build and Makefile
On Fri, Jun 16, 2023 at 1:56 PM Tristan Partin wrote: > > On Fri Jun 16, 2023 at 3:29 PM CDT, Jeff Davis wrote: > > Patch attached. Currently, the Makefile specifies NO_LOCALE=1, and the > > meson.build does not. > > Looks alright to me, but it might be nicer to change the order of > arguments to match contrib/unaccent/meson.build:40. Might help with > grepping in the future. It seems that Jeff's patch tried to match the precedent set in src/test/modules/test_oat_hooks/meson.build. No matter which ordering Jeff's patch uses, it will be inconsistent with one of the existing order of the options. So attached is updated patch that makes the order consistent across all 3 occurrences. Best regards, Gurjeet http://Gurje.et v2-0001-test_extensions-make-meson.build-consistent-with-.patch Description: Binary data
Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c)
On Wed, Jun 14, 2023 at 5:12 AM Ranier Vilela wrote: > > Em qua., 14 de jun. de 2023 às 06:51, Richard Guo > escreveu: >> >> >> On Tue, Jun 13, 2023 at 3:39 PM Kyotaro Horiguchi >> wrote: >>> >>> Gurjeet has mentioned that eb.rel cannot be modified by another >>> process since the value or memory is in the local stack, and I believe >>> he's correct. >>> >>> If the pointed Relation had been blown out, eb.rel would be left >>> dangling, not nullified. However, I don't believe this situation >>> happens (or it shouldn't happen) as the entire relation should already >>> be locked. >> >> >> Yeah, Gurjeet is right. I had a thinko here. eb.rel should not be NULL >> pointer in any case. And as we've acquired the lock for it, it should >> not have been closed. So I think we can remove the check for eb.rel in >> the two places. > > Ok, > As there is a consensus on removing the tests and the comment is still > relevant, > here is a new version for analysis. LGTM. Best regards, Gurjeet http://Gurje.et
Re: Document that server will start even if it's unable to open some TCP/IP ports
On Mon, Jun 12, 2023 at 10:59 PM Nathan Bossart wrote: > On Sat, May 27, 2023 at 03:17:21PM -0700, Gurjeet Singh wrote: > > If listen_addresses is empty, then server won't try to open any TCP/IP > > ports. The patch does not change any language related to that. > > Your proposed change notes that the server only starts if it can listen on > at least one TCP/IP address, which I worry might lead folks to think that > the server won't start if listen_addresses is empty. Perhaps we can prefix that statement with "If listen_addresses is not empty", like so: --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -661,3 +661,9 @@ include_dir 'conf.d' which allows only local TCP/IP loopback connections to be - made. While client authentication (listen_addresses is not empty, the server + will start only if it can open the port + on at least one TCP/IP address. If server is unable to open + port on a TCP/IP address, it emits a warning. + + + While client authentication () allows fine-grained control Best regards, Gurjeet http://Gurje.et
Re: RFC: Adding \history [options] [filename] to psql (Snippets and Shared Queries)
On Mon, Jun 5, 2023 at 8:58 AM Kirk Wolak wrote:
>
> Everyone,
> After recently deep diving on some readline features and optimizing my bash
> environment to have a static set of "snippets" that I can always find...
>
> it takes just a couple of history API calls to add some interesting
> features for those that want them. The result of adding 3-4 such commands
> (all under \history, and with compatible flags):
>
> - Saving your current history without exiting (currently doable as \s
> :HISTFILE)
> - Reloading your history file (so you can easily share something across
> sessions) w/o exiting.
> - Stack Loading of specific history (like a shared snippets library, and a
> personal snippets library) [clearing your history, then loading them in a
> custom order]
>
> The upside is really about clearly identifying and sharing permanent
> snippets, while having that list be editable externally. Again, bringing
> teams online who don't always know the PG way of doing things (Waits, Locks,
> Space, High CPU queries, Running Queries).
>
> My intention is to leverage the way PSQL keeps the Comment above the SQL
> with the SQL.
> Then I can step backwards searching for "context" markers (Ctrl-R) or
> -- [F8] {history-search-backward}
>
> To rip through my snippets
>
> Kirk...
> PS: I could do all of this under \s [options] [filename] it's just less
> clear...
Understandably, there doesn't seem to be a lot of enthusiasm for this.
If you could show others a sample/demo session of what the UI and UX
would look like, maybe others can chime in with either their opinion
of the behaviour, or perhaps a better/different way of achieving that.
Best regards,
Gurjeet
http://Gurje.et
Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c)
On Mon, Jun 5, 2023 at 4:24 AM Ranier Vilela wrote:
> Em seg., 5 de jun. de 2023 às 08:06, Ranier Vilela
> escreveu:
>> Em dom., 4 de jun. de 2023 às 23:37, Richard Guo
>> escreveu:
>>> On Sun, Jun 4, 2023 at 8:42 PM Ranier Vilela wrote:
Hi,
Per Coverity.
At function ExtendBufferedRelShared, has a always true test.
eb.rel was dereferenced one line above, so in
if (eb.rel) is always true.
I think it's worth removing the test, because Coverity raises dozens of
alerts thinking eb.rel might be NULL.
Besides, one less test is one less branch.
>>>
>>>
>>> This also happens in ExtendBufferedRelTo, and the comment there explains
>>> that the eb.rel 'could have been closed while waiting for lock'.
>>
>> Well, RelationGetSmgr also dereferences eb.rel.
>> If eb.rel could be closed while waiting for lock,
>> anyone who references eb.rel below takes a risk?
>>
>> static inline SMgrRelation
>> RelationGetSmgr(Relation rel)
>> {
>> if (unlikely(rel->rd_smgr == NULL))
>> smgrsetowner(&(rel->rd_smgr), smgropen(rel->rd_locator, rel->rd_backend));
>> return rel->rd_smgr;
>> }
>
> Sorry Richard, nevermind.
>
> My fault, I withdraw this patch.
I'm not quite convinced of the reasoning provided; either the reason
is not good enough, or my C is rusty. In either case, I'd like a
resolution.
The code in question:
> LockRelationForExtension(eb.rel, ExclusiveLock);
>
> /* could have been closed while waiting for lock */
> if (eb.rel)
> eb.smgr = RelationGetSmgr(eb.rel);
eb.rel is being passed by-value at line 1, so even if the relation is
closed, the value of the eb.rel cannot change between line 1 and line
3. So a code verification tool complaining that the 'if' condition
will always be true is quite right, IMO.
To verify my assumptions, I removed those checks and ran `make
{check,check-world,installcheck}`, and all those tests passed.
The only way, that I can think of, the value of eb.rel can change
between lines 1 and 3 is if 'eb' is a shared-memory data structure,
and some other process changed the 'rel' member in shared-memory. And
I don't think 'eb' is in shared memory in this case.
To me, it looks like these checks are a result of code being
copy-pasted from somewhere else, where this check might have been
necessary. The checks are sure not necessary at these spots.
Please see attached v2 of the patch; it includes both occurrences of
the spurious checks identified in this thread.
Best regards,
Gurjeet
http://Gurje.et
v2-0001-Remove-always-true-checks.patch
Description: Binary data
Re: Use COPY for populating all pgbench tables
On Fri, Jun 9, 2023 at 6:20 PM Gurjeet Singh wrote: > On Fri, Jun 9, 2023 at 5:42 PM Gregory Smith wrote: > > On Fri, Jun 9, 2023 at 1:25 PM Gurjeet Singh wrote: > >> > >> > $ pgbench -i -I dtGvp -s 500 > >> > >> The steps are severely under-documented in pgbench --help output. > > > > I agree it's not easy to find information. I just went through double > > checking I had the order recently enough to remember what I did. The man > > pages have this: > > > > > Each step is invoked in the specified order. The default is dtgvp. > > > > Which was what I wanted to read. Meanwhile the --help output says: > > > > > -I, --init-steps=[dtgGvpf]+ (default "dtgvp") > > > > %T$%%Which has the information without a lot of context for what it's used > > for. I'd welcome some text expansion that added a minimal but functional > > improvement to that the existing help output; I don't have such text. > > Please see attached 2 variants of the patch. Variant 1 simply tells > the reader to consult pgbench documentation. The second variant > provides a description for each of the letters, as the documentation > does. The committer can pick the one they find suitable. (I was short on time, so had to keep it short in the above email. To justify this additional email, I have added 2 more options to choose from. :) The text ", in the specified order" is an important detail, that should be included irrespective of the rest of the patch. My preference would be to use the first variant, since the second one feels too wordy for --help output. I believe we'll have to choose between these two; the alternatives will not make anyone happy. These two variants show the two extremes; bare minimum vs. everything but the kitchen sink. So one may feel the need to find a middle ground and provide a "sufficient, but not too much" variant. I have attempted that in variants 3 and 4; see attached. The third variant does away with the list of steps, and uses a paragraph to describe the letters. And the fourth variant makes that paragraph terse. In the order of preference I'd choose variant 1, then 2. Variants 3 and 4 feel like a significant degradation from variant 2. Attached samples.txt shows the snippets of --help output of each of the variants/patches, for comparison. Best regards, Gurjeet http://Gurje.et variant-1-brief-pointer-to-docs.patch -i, --initialize invokes initialization mode -I, --init-steps=[dtgGvpf]+ (default "dtgvp") run selected initialization steps, in the specified order see pgbench documentation for a description of these steps -F, --fillfactor=NUM set fill factor variant-2-detailed-description.patch -i, --initialize invokes initialization mode -I, --init-steps=[dtgGvpf]+ (default "dtgvp") run selected initialization steps, in the specified order d: Drop any existing pgbench tables t: Create the tables used by the standard pgbench scenario g: Generate data, client-side G: Generate data, server-side v: Invoke VACUUM on the standard tables p: Create primary key indexes on the standard tables f: Create foreign key constraints between the standard tables -F, --fillfactor=NUM set fill factor variant-3-details-not-list.patch -i, --initialize invokes initialization mode -I, --init-steps=[dtgGvpf]+ (default "dtgvp") run selected initialization steps, in the specified order. These steps operate on standard tables used by pgbench 'd' drops the tables, 't' creates the tables, 'g' generates the data on client-side, 'G' generates data on server-side, 'v' vaccums the tables, 'p' creates primary key constraints, and 'f' creates foreign key constraints -F, --fillfactor=NUM set fill factor variant-4-terse-details-not-list.patch -i, --initialize invokes initialization mode -I, --init-steps=[dtgGvpf]+ (default "dtgvp") run selected initialization steps, in the specified order. These steps operate on standard tables used by pgbench 'd' drop table, 't' create tables, 'g' generate data client-side, 'G' generate data server-side, 'v' vaccum tables, 'p' create primary keys, 'f' create foreign keys -F, --fillfactor=NUM set fill factor diff --git
Re: Use COPY for populating all pgbench tables
On Fri, Jun 9, 2023 at 5:42 PM Gregory Smith wrote: > > On Fri, Jun 9, 2023 at 1:25 PM Gurjeet Singh wrote: >> >> > $ pgbench -i -I dtGvp -s 500 >> >> The steps are severely under-documented in pgbench --help output. > > > I agree it's not easy to find information. I just went through double > checking I had the order recently enough to remember what I did. The man > pages have this: > > > Each step is invoked in the specified order. The default is dtgvp. > > Which was what I wanted to read. Meanwhile the --help output says: > > > -I, --init-steps=[dtgGvpf]+ (default "dtgvp") > > %T$%%Which has the information without a lot of context for what it's used > for. I'd welcome some text expansion that added a minimal but functional > improvement to that the existing help output; I don't have such text. Please see attached 2 variants of the patch. Variant 1 simply tells the reader to consult pgbench documentation. The second variant provides a description for each of the letters, as the documentation does. The committer can pick the one they find suitable. Best regards, Gurjeet http://Gurje.et variant-2-detailed-description.patch Description: Binary data variant-1-brief-pointer-to-docs.patch Description: Binary data
Re: Fix search_path for all maintenance commands
On Fri, Jun 9, 2023 at 2:00 PM Jeff Davis wrote: > > On Thu, 2023-06-08 at 21:55 -0700, Nathan Bossart wrote: > > On Thu, Jun 08, 2023 at 06:08:08PM -0400, Greg Stark wrote: > > > I guess that's pretty narrow and a reasonable thing to desupport. > > > Users could just mark those functions with search_path or schema > > > qualify the object references in them. Perhaps we should also be > > > picking up cases like that sooner so users realize they've created > > > a > > > footgun for themselves? > > Many cases will be picked up, for instance CREATE INDEX will error if > the safe search path is not good enough. > > > I'm inclined to agree that this is reasonable to desupport. > > Committed. I'm not sure if mine is a valid concern, and it has been a long time since I looked at the search_path's and switching Role's implications (CVE-2009-4136) so pardon my ignorance. It feels a bit late in release cycle to introduce this breaking change. If they depended on search_path, people and utilities that use these maintenance commands may see failures. Although I can't think of a scenario where such a failure may cause an outage, sometimes these maintenance operations are performed while the users are staring down the barrel of a gun (imminent danger of running out of space, bad statistics causing absurd query plans, etc.). So, if not directly, a failure of these operations may indirectly cause an outage. I feel more thought needs to be given to the impact of this change, and we should to give others more time for feedback. Short of that, it'd be prudent to allow the user to somehow fall back to old behaviour; a command-line option, or GUC, etc. That way we can mark the old behaviour "deprecated", with a workaround for those who may desperately need it, and in another release or so, finally pull the plug on old behaviour. My 2bits. Best regards, Gurjeet http://Gurje.et
Re: Use COPY for populating all pgbench tables
On Fri, Jun 9, 2023 at 6:24 AM Gregory Smith wrote: > > Unfortunately there's no simple command line option to change just that one > thing about how pgbench runs. You have to construct a command line that > documents each and every step you want instead. You probably just want this > form: > > $ pgbench -i -I dtGvp -s 500 The steps are severely under-documented in pgbench --help output. Grepping that output I could not find any explanation of these steps, so I dug through the code and found them in runInitSteps(). Just as I was thinking of writing a patch to remedy that, just to be sure, I checked online docs and sure enough, they are well-documented under pgbench [1]. I think at least a pointer to the the pgbench docs should be mentioned in the pgbench --help output; an average user may not rush to read the code to find the explanation, but a hint to where to find more details about what the letters in --init-steps mean, would save them a lot of time. [1]: https://www.postgresql.org/docs/15/pgbench.html Best regards, Gurjeet http://Gurje.et
Re: Major pgbench synthetic SELECT workload regression, Ubuntu 23.04+PG15
On Fri, Jun 9, 2023 at 12:28 AM Gregory Smith wrote: > > Let me start with the happy ending to this thread: Phew! I'm sure everyone would be relieved to know this was a false alarm. > On Thu, Jun 8, 2023 at 9:21 PM Andres Freund wrote: >> >> You might need to add --no-children to the perf report invocation, otherwise >> it'll show you the call graph inverted. > > > My problem was not writing kernel symbols out, I was only getting addresses > for some reason. This worked: > > sudo perf record -g --call-graph dwarf -d --phys-data -a sleep 1 > perf report --stdio There is no mention of perf or similar utilities in pgbench-tools docs. I'm guessing Linux is the primary platform pgbench-tools gets used on most. If so, I think it'd be useful to mention these tools and snippets in there to make others lives easier, when they find themselves scratching heads. Best regards, Gurjeet http://Gurje.et
Re: Error in calculating length of encoded base64 string
On Thu, Jun 8, 2023 at 7:35 AM Tom Lane wrote: > > o.tselebrovs...@postgrespro.ru writes: > > While working on an extension I've found an error in how length of > > encoded base64 string is calulated; > > Yeah, I think you're right. It's not of huge significance, because > it just overestimates by 1 or 2 bytes, but we might as well get > it right. Thanks for the report and patch! >From your commit d98ed080bb >This bug is very ancient, dating to commit 79d78bb26 which >added encode.c. (The other instances were presumably copied >from there.) Still, it doesn't quite seem worth back-patching. Is it worth investing time in trying to unify these 3 occurrences of base64 length (and possibly other relevant) code to one place? If yes, I can volunteer for it. The common code facility under src/common/ did not exist back when pgcrypto was added, but since it does now, it may be worth it make others depend on implementation in src/common/ code. Best regards, Gurjeet http://Gurje.et
Re: Fix last unitialized memory warning
On Wed, Jun 7, 2023 at 7:31 AM Tristan Partin wrote:
>
> This patch is really not necessary from a functional point of view. It
> is only necessary if we want to silence a compiler warning.
>
> Tested on `gcc (GCC) 13.1.1 20230511 (Red Hat 13.1.1-2)`.
...
> From my perspective, this warning definitely seems like a false
> positive, but I don't know the code well-enough to say that for certain.
It was a bit confusing to see a patch for src/bin/pgbench/pgbench.c,
but that file not mentioned in the warning messages you quoted. I take
it that your patch silences a warning in pgbench.c; it would've been
nice to see the actual warning.
-PgBenchValue vargs[MAX_FARGS];
+PgBenchValue vargs[MAX_FARGS] = { 0 };
If I'm right about what kind of warning this might've caused (use of
possibly uninitialized variable), you're correct that it is benign.
The for loop after declarations initializes all the elements of this
array using evaluateExpr(), and if the initialization fails for some
reason, the loop ends prematurely and returns from the function.
I analyzed a few code-paths that return true from evaluateExpr(), and
I'd like to believe that _all_ code paths that return true also
initialize the array element passed. But because there are so many
branches and function calls beneath evaluateExpr(), I think it's
better to be paranoid and initialize all the array elements to 0.
Also, it is better to initialize/clear an array at the point of
definition, like your patch does. So, +1 to the patch.
Best regards,
Gurjeet
http://Gurje.et
Re: Typo in src/backend/access/nbtree/README?
On Thu, Jun 8, 2023 at 7:11 AM Daniel Westermann (DWE) wrote: > > ... shouldn't there be a "to" before "detect"? > > These two additions make it possible detect a concurrent page split Agreed. Attached is a small patch that fixes this. Thanks for the report! Best regards, Gurjeet http://Gurje.et From f694a97e72a3ad744073c9a04f3c39615b1a9987 Mon Sep 17 00:00:00 2001 From: Gurjeet Singh Date: Thu, 8 Jun 2023 19:30:57 -0700 Subject: [PATCH v1] Fix grammar As reported by Daniel Westermann. --- src/backend/access/nbtree/README | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/access/nbtree/README b/src/backend/access/nbtree/README index dd0f7ad2bd..1174ab9131 100644 --- a/src/backend/access/nbtree/README +++ b/src/backend/access/nbtree/README @@ -17,7 +17,7 @@ The basic Lehman & Yao Algorithm Compared to a classic B-tree, L adds a right-link pointer to each page, to the page's right sibling. It also adds a "high key" to each page, which is an upper bound on the keys that are allowed on that page. These two -additions make it possible detect a concurrent page split, which allows the +additions make it possible to detect a concurrent page split, which allows the tree to be searched without holding any read locks (except to keep a single page from being modified while reading it). -- 2.35.1
Re: Mark a transaction uncommittable
On Sat, Apr 22, 2023 at 4:33 PM Julien Rouhaud wrote: > > Hi, > > On Sat, Apr 22, 2023 at 12:53:23PM -0400, Isaac Morland wrote: > > > > I have an application for this: creating various dev/test versions of data > > from production. > > > > Start by restoring a copy of production from backup. Then successively > > create several altered versions of the data and save them to a place where > > developers can pick them up. For example, you might have one version which > > has all data old than 1 year deleted, and another where 99% of the > > students/customers/whatever are deleted. Anonymization could also be > > applied. This would give you realistic (because it ultimately originates > > from production) test data. > > > > This could be done by starting a non-committable transaction, making the > > adjustments, then doing a pg_dump in the same transaction (using --snapshot > > to allow it to see that transaction). Then rollback, and repeat for the > > other versions. This saves repeatedly restoring the (probably very large) > > production data each time. > > There already are tools to handle those use cases. Looks for instance at > https://github.com/mla/pg_sample to backup a consistent subset of the data, or > https://github.com/rjuju/pg_anonymize to transparently pg_dump (or > interactively query) anonymized data. > > Both tool also works when connected on a physical standby, while trying to > update data before dumping them wouldn't. Like everything in life, there are pros and cons to every approach. pg_anonymize is an extension that may not be installed on the database you're working with. And pg_sample (and similar utilities) may not have a way to extract or sanitize the exact data you want. With this feature built into Postgres, you'd not need any external utilities or extensions. The benefits of features built into Postgres are that the users can come up with ways of leveraging such a feature in future in a way that we can't envision today. Best regards, Gurjeet http://Gurje.et
Re: Mark a transaction uncommittable
On Mon, Jun 5, 2023 at 12:32 AM Laurenz Albe wrote: > > On Mon, 2023-06-05 at 00:22 -0700, Gurjeet Singh wrote: > > On Sat, Apr 22, 2023 at 8:01 AM Gurjeet Singh wrote: > > > > > > This is a proposal for a new transaction characteristic. I haven't > > > written any code, yet, and am interested in hearing if others may find > > > this feature useful. > > > > Please see attached the patch that introduces this new feature. > > Can you explain why *you* would find this feature useful? The idea came to me while I was reading a blog post, where the author had to go to great lengths to explain to the reader why the queries would be disastrous, if run on production database, and that they should run those queries inside a transaction, and they _must_ rollback the transaction. Having written my fair share of tutorials, and blogs, I know how helpful it would be to start a transaction that the reader can't accidentally commit. As others have noted in this thread, this feature can be useful in other situations, as well, like when you're trying to export a sanitized copy of a production database. Especially in such a situation you do not want those sanitization operations to ever be committed on the source database. Best regards, Gurjeet http://Gurje.et
Re: Mark a transaction uncommittable
On Sat, Apr 22, 2023 at 8:01 AM Gurjeet Singh wrote: > > This is a proposal for a new transaction characteristic. I haven't > written any code, yet, and am interested in hearing if others may find > this feature useful. Please see attached the patch that introduces this new feature. The patch includes all the code changes that I could foresee; that is, it includes server changes as well as changes for psql's auto-completion. The patch does not include doc changes, or any tests. Please see a sample session at the end of the email, though. The patch introduces a new keyword, COMMITTABLE, and uses that to introduce the new transaction attribute via BEGIN [TRANSACTION] and SET TRANSACTION commands. The new code follows the semantics of the [NOT] DEFERRABLE attribute of a transaction almost exactly. > Many a times we start a transaction that we never intend to commit; > for example, for testing, or for EXPLAIN ANALYZE, or after detecting > unexpected results but still interested in executing more commands > without risking commit, etc. > > A user would like to declare their intent to eventually abort the > transaction as soon as possible, so that the transaction does not > accidentally get committed. > > This feature would allow the user to mark a transaction such that it > can never be committed. We must allow such marker to be placed when > the transaction is being started, or while it's in progress. The user can mark the transaction as uncommittable either when starting the transaction, or while it is still in progress. > Once marked uncommittable, do not allow the marker to be removed. > Hence, once deemed uncommittable, the transaction cannot be committed, > even intentionally. This protects against cases where one script > includes another (e.g. psql's \i command), and the included script may > have statements that turn this marker back on. Although the patch implements this desired behavior from the initial proposal, I'm no longer convinced of the need to prevent user from re-enabling committability of the transaction. > Any command that ends a transaction (END, COMMIT, ROLLBACK) must > result in a rollback. > > All of these properties seem useful for savepoints, too. But I want to > focus on just the top-level transactions, first. The patch does not change any behaviour related to savepoints. Having made it work for the top-level transaction, and having seen the savepoint/subtransaction code as I came across it as I developed this patch, I feel that it will be very tricky to implement this behavior safely for savepoints. Moreover, having thought more about the possible use cases, I don't think implementing uncommittability of savepoints will be of much use in the real world. > I feel like the BEGIN and SET TRANSACTION commands would be the right > places to introduce this feature. > > BEGIN [ work | transaction ] [ [ NOT ] COMMITTABLE ]; > SET TRANSACTION [ [ NOT ] COMMITTABLE ]; I tried to avoid adding a new keyword (COMMITTABLE) to the grammar, but could not think of a better alternative. E.g. DISALLOW COMMIT sounds like a good alternative, but DISALLOW is currently not a keyword, so this form doesn't buy us anything. > I'm not yet sure if the COMMIT AND CHAIN command should carry this > characteristic to the next transaction. After a little consideration, in the spirit of POLA, I have not done anything special to change the default behaviour of COMMIT/ROLLBACK AND CHAIN. Any feedback is welcome. Please see below an example session demonstrating this feature. postgres=# begin transaction committable; BEGIN postgres=# commit; COMMIT postgres=# begin transaction not committable; BEGIN postgres=# commit; WARNING: transaction is not committable ROLLBACK postgres=# begin transaction not committable; BEGIN postgres=# set transaction_committable = true; -- for clarity, we may want to emit additional "WARNING: cannot make transaction committable", although the patch currently doesn't do so. ERROR: invalid value for parameter "transaction_committable": 1 postgres=# commit; ROLLBACK postgres=# begin transaction not committable; BEGIN postgres=# set transaction committable ; -- for clarity, we may want to emit additional "WARNING: cannot make transaction committable", although the patch currently doesn't do so. ERROR: invalid value for parameter "transaction_committable": 1 postgres=# set transaction committable ; ERROR: current transaction is aborted, commands ignored until end of transaction block postgres=# commit; ROLLBACK Best regards, Gurjeet http://Gurje.et v1-0001-Allow-user-to-mark-a-transaction-uncommittable.patch Description: Binary data
Document that server will start even if it's unable to open some TCP/IP ports
The attached patch clarifies that the server will start even if it is unable to open the port on some of the TCP/IP addresses listed (or implied by a value of '*' or localhost) in listen_addresses parameter. I think it is important to call this out, because I was surprised to see that the server started even though the port was occupied by another process. Upon close inspection, I noticed that the other process was using that port on 127.0.0.1, so Postgres complained about that interface (a warning in server log), but it was able to open the port on IPv6 ::1, so it started up as normal. Upon further testing, I saw that server will not start only if it is unable to open the port on _all_ the interfaces/addresses. It it's able to open the port on at least one, the server will start. If listen_addresses is empty, then server won't try to open any TCP/IP ports. The patch does not change any language related to that. Best regards, Gurjeet http://Gurje.et v1-0001-Document-that-the-server-starts-only-if-at-least-.patch Description: Binary data
Postgres Index Advisor status and GSoC (was: Re: Returning to Postgres community work)
On Tue, Aug 31, 2021 at 10:02 AM Gurjeet Singh wrote: > On Tue, Aug 31, 2021 at 8:04 AM Alvaro Herrera > wrote: > > > You know what I've heard? That your index advisor module languishes > > unmaintained and that there's no shortage of people wishing to use it. > > Now there's a masterclass in making someone feel great and guilty in > the same sentence ;-) > > > Heck, we spent a lot of back-and-forth in the spanish mailing list > > with somebody building a super obsolete version of Postgres just so that > > they could compile your index advisor. I dunno, if you have some spare > > time, maybe updating that one would be a valuable contribution from > > users' perspective. > > Aye-aye Capn' :-) As part of helping the GSoC contributor getting onboard (see below), I went through a similar process and had to figure out the Postgres version, system packages, etc. (all ancient) that were needed to build and use it. It's no fun having to deal with software from over a decade ago :-( > EDB folks reached out to me a few months ago to assign a license to > the project, which I did and it is now a Postgres-licensed project > [1]. > > Given the above, it is safe to assume that this tool is at least being > maintained by EDB, at least internally for their customers. I would > request them to contribute the changes back in the open. After over a year of conversations and follow-ups, a couple of months ago EnterpriseDB finally made it clear that they won't be contributing their changes back to the open-source version of Index Advisor. With that avenue now closed, we can now freely pursue > Regardless of that, please rest assured that I will work on making it > compatible with the current supported versions of Postgres. Lack of > time is not an excuse anymore :-) Oh, how wrong was I :-) I have a few updates on the current state and plans around the Index Adviser extension. I proposed Index Adviser as a potential project for GSoC 2023 [1]. Ahmed (CCd) has signed up as the contributor. The project has now been accepted/funded by GSoC. The primary goal of the project is to support all the active versions of Postgres. The extended goal is to support additional index types. The extension currently supports Postgres version 8.3, and BTree index type. [1]: https://wiki.postgresql.org/wiki/GSoC_2023#pg_adviser_.2F_index_adviser:_Recommend_Potentially_Useful_Indexes Best regards, Gurjeet http://Gurje.et
Re: Minor code de-duplication in fe-connect.c
On Mon, Apr 24, 2023 at 5:14 AM Daniel Gustafsson wrote: > > On 21 Apr 2023, at 18:38, Gurjeet Singh wrote: > > > > On Fri, Apr 21, 2023 at 7:47 AM Robert Haas > wrote: > >> > >> On Fri, Apr 21, 2023 at 8:25 AM Daniel Gustafsson > wrote: > >>> The reason I left it like this when reviewing and committing is that I > think it > >>> makes for more readable code. The amount of lines saved is pretty > small, and > >>> "shuffle" isn't an exact term so by reading the code it isn't > immediate clear > >>> what such a function would do. By having the shuffle algorithm where > it's used > >>> it's clear what the code does and what the outcome is. If others > disagree I > >>> can go ahead and refactor of course, but I personally would not deem > it a net > >>> win in code quality. > >> > >> I think we should avoid nitpicking stuff like this. I likely would > >> have used a subroutine if I'd done it myself, but I definitely > >> wouldn't have submitted a patch to change whatever the last person did > >> without some tangible reason for so doing. > > > > Code duplication, and the possibility that a change in one location > > (bugfix or otherwise) does not get applied to the other locations, is > > a good enough reason to submit a patch, IMHO. > > > >> It's not a good use of > >> reviewer and committer time to litigate things like this. > > > > Postgres has a very high bar for code quality, and for documentation. > > It is a major reason that attracts people to, and keeps them in the > > Postgres ecosystem, as users and contributors. If anything, we should > > encourage folks to point out such inconsistencies in code and docs > > that keep the quality high. > > > > This is not a attack on any one commit, or author/committer of the > > commit; sorry if it appeared that way. I was merely reviewing the > > commit that introduced a nice libpq feature. This patch is merely a > > minor improvement to the code that I think deserves a consideration. > > It's not a litigation, by any means. > > I didn't actually read the patch earlier, but since Robert gave a +1 to the > idea of refactoring I had a look. I have a few comments: > > +static void > +shuffleAddresses(PGConn *conn) > This fails to compile since the struct is typedefed PGconn. > > > - /* > -* This is the "inside-out" variant of the Fisher-Yates shuffle > -* algorithm. Notionally, we append each new value to the array > -* and then swap it with a randomly-chosen array element (possibly > -* including itself, else we fail to generate permutations with > -* the last integer last). The swap step can be optimized by > -* combining it with the insertion. > -* > * We don't need to initialize conn->prng_state here, because that > * already happened in connectOptions2. > */ > This also fails to compile since it removes the starting /* marker of the > comment resulting in a comment starting on *. > > > - for (i = 1; i < conn->nconnhost; i++) > - for (int i = 1; i < conn->naddr; i++) > You are replacing these loops for shuffling an array with a function that > does > this: > + for (int i = 1; i < conn->naddr; i++) > This is not the same thing, one is shuffling the hosts and the other is > shuffling the addresses resolved for the hosts (which may be a n:m > relationship). If you had compiled and run the tests you would have seen > that > the libpq tests now fail with this applied, as would be expected. > > > Shuffling the arrays can of course be made into a subroutine, but what to > shuffle and where needs to be passed in to it and at that point I doubt the > code readability is much improved over the current coding. > > Sorry about the errors. This seems to be the older version of the patch that I had generated before fixing these mistakes. I do remember encountering the compiler errors and revising the code to fix these, especially the upper vs. lower case and the partial removal of the coment. Away from my keyboard, so please expect the newer patch some time later. Best regards, Gurjeet http://Gurje.et
Re: Doc limitation update proposal: include out-of-line OID usage per TOAST-ed columns
On Fri, Apr 21, 2023 at 12:14 AM Nikita Malakhov wrote: > This limitation applies not only to wide tables - it also applies to tables > where TOASTed values > are updated very often. You would soon be out of available TOAST value ID > because in case of > high frequency updates autovacuum cleanup rate won't keep up with the > updates. It is discussed > in [1]. > > On Fri, Apr 21, 2023 at 9:39 AM Jakub Wartak > wrote: >> I would like to ask if it wouldn't be good idea to copy the >> https://wiki.postgresql.org/wiki/TOAST#Total_table_size_limit >> discussion (out-of-line OID usage per TOAST-ed columns / potential >> limitation) to the official "Appendix K. PostgreSQL Limits" with also >> little bonus mentioning the "still searching for an unused OID in >> relation" notice. Although it is pretty obvious information for some >> and from commit 7fbcee1b2d5f1012c67942126881bd492e95077e and the >> discussion in [1], I wonder if the information shouldn't be a little >> more well known via the limitation (especially to steer people away >> from designing very wide non-partitioned tables). >> >> [1] - >> https://www.postgresql.org/message-id/flat/16722-93043fb459a41073%40postgresql.org > > [1] > https://www.postgresql.org/message-id/CAN-LCVPRvRzxeUdYdDCZ7UwZQs1NmZpqBUCd%3D%2BRdMPFTyt-bRQ%40mail.gmail.com These 2 discussions show that it's a painful experience to run into this problem, and that the hackers have ideas on how to fix it, but those fixes haven't materialized for years. So I would say that, yes, this info belongs in the hard-limits section, because who knows how long it'll take this to be fixed. Please submit a patch. I anticipate that edits to Appendix K Postgres Limits will prompt improving the note in there about the maximum column limit, That note is too wordy, and sometimes confusing, especially for the audience that it's written for: newcomers to Postgres ecosystem. Best regards, Gurjeet https://Gurje.et http://aws.amazon.com
Re: [PATCH] Infinite loop while acquiring new TOAST Oid
On Thu, Dec 22, 2022 at 10:07 AM Nikita Malakhov wrote: > Any suggestions on the previous message (64-bit toast value ID with > individual counters)? Was this patch ever added to CommitFest? I don't see it in the current Open Commitfest. https://commitfest.postgresql.org/43/ Best regards, Gurjeet http://Gurje.et http://aws.amazon.com
Mark a transaction uncommittable
This is a proposal for a new transaction characteristic. I haven't written any code, yet, and am interested in hearing if others may find this feature useful. Many a times we start a transaction that we never intend to commit; for example, for testing, or for EXPLAIN ANALYZE, or after detecting unexpected results but still interested in executing more commands without risking commit, etc. A user would like to declare their intent to eventually abort the transaction as soon as possible, so that the transaction does not accidentally get committed. This feature would allow the user to mark a transaction such that it can never be committed. We must allow such marker to be placed when the transaction is being started, or while it's in progress. Once marked uncommittable, do not allow the marker to be removed. Hence, once deemed uncommittable, the transaction cannot be committed, even intentionally. This protects against cases where one script includes another (e.g. psql's \i command), and the included script may have statements that turn this marker back on. Any command that ends a transaction (END, COMMIT, ROLLBACK) must result in a rollback. All of these properties seem useful for savepoints, too. But I want to focus on just the top-level transactions, first. I feel like the BEGIN and SET TRANSACTION commands would be the right places to introduce this feature. BEGIN [ work | transaction ] [ [ NOT ] COMMITTABLE ]; SET TRANSACTION [ [ NOT ] COMMITTABLE ]; I'm not yet sure if the COMMIT AND CHAIN command should carry this characteristic to the next transaction. Thoughts? Best regards, Gurjeet https://Gurje.et http://aws.amazon.com
Re: Correct the documentation for work_mem
On Fri, Apr 21, 2023 at 10:15 AM Tom Lane wrote: > > Peter Eisentraut writes: > > On 21.04.23 16:28, Imseih (AWS), Sami wrote: > >> I suggest a small doc fix: > >> “Note that for a complex query, several sort or hash operations might be > >> running simultaneously;” > > > Here is a discussion of these terms: > > https://takuti.me/note/parallel-vs-concurrent/ > > > I think "concurrently" is the correct word here. > > Probably, but it'd do little to remove the confusion Sami is on about, +1. When discussing this internally, Sami's proposal was in fact to use the word 'concurrently'. But given that when it comes to computers and programming, it's common for someone to not understand the intricate difference between the two terms, we thought it's best to not use any of those, and instead use a word not usually associated with programming and algorithms. Aside: Another pair of words I see regularly used interchangeably, when in fact they mean different things: precise vs. accurate. > especially since the next sentence uses "concurrently" to describe the > other case. I think we need a more thorough rewording, perhaps like > > - Note that for a complex query, several sort or hash operations might > be > - running in parallel; each operation will generally be allowed > + Note that a complex query may include several sort or hash > + operations; each such operation will generally be allowed This wording doesn't seem to bring out the fact that there could be more than one work_mem consumer running (in-progress) at the same time. The reader to could mistake it to mean hashes and sorts in a complex query may happen one after the other. + Note that a complex query may include several sort and hash operations, and + more than one of these operations may be in progress simultaneously at any + given time; each such operation will generally be allowed I believe the phrase "several sort _and_ hash" better describes the possible composition of a complex query, than does "several sort _or_ hash". > I also find this wording a bit further down to be poor: > > Hash-based operations are generally more sensitive to memory > availability than equivalent sort-based operations. The > memory available for hash tables is computed by multiplying > work_mem by > hash_mem_multiplier. This makes it > > I think "available" is not le mot juste, and it's also unclear from > this whether we're speaking of the per-hash-table limit or some > (nonexistent) overall limit. How about > > - memory available for hash tables is computed by multiplying > + memory limit for a hash table is computed by multiplying +1 Best regards, Gurjeet https://Gurje.et Postgres Contributors Team, http://aws.amazon.com
Re: Minor code de-duplication in fe-connect.c
On Fri, Apr 21, 2023 at 7:47 AM Robert Haas wrote: > > On Fri, Apr 21, 2023 at 8:25 AM Daniel Gustafsson wrote: > > The reason I left it like this when reviewing and committing is that I > > think it > > makes for more readable code. The amount of lines saved is pretty small, > > and > > "shuffle" isn't an exact term so by reading the code it isn't immediate > > clear > > what such a function would do. By having the shuffle algorithm where it's > > used > > it's clear what the code does and what the outcome is. If others disagree I > > can go ahead and refactor of course, but I personally would not deem it a > > net > > win in code quality. > > I think we should avoid nitpicking stuff like this. I likely would > have used a subroutine if I'd done it myself, but I definitely > wouldn't have submitted a patch to change whatever the last person did > without some tangible reason for so doing. Code duplication, and the possibility that a change in one location (bugfix or otherwise) does not get applied to the other locations, is a good enough reason to submit a patch, IMHO. > It's not a good use of > reviewer and committer time to litigate things like this. Postgres has a very high bar for code quality, and for documentation. It is a major reason that attracts people to, and keeps them in the Postgres ecosystem, as users and contributors. If anything, we should encourage folks to point out such inconsistencies in code and docs that keep the quality high. This is not a attack on any one commit, or author/committer of the commit; sorry if it appeared that way. I was merely reviewing the commit that introduced a nice libpq feature. This patch is merely a minor improvement to the code that I think deserves a consideration. It's not a litigation, by any means. Best regards, Gurjeet https://Gurje.et Postgres Contributors Team, http://aws.amazon.com
Re: Reorder connection markers in libpq TAP tests
On Thu, Apr 20, 2023 at 11:00 PM Gurjeet Singh wrote:
>
> Commit 7f5b198 introduced TAP tests that use string literals to mark
> the presence of a query in server logs. For no explicable reason, the
> tests with the marker 'connect2' occur before the tests that use
> 'connect1' marker.
>
> The attached patch swaps the connection marker strings so that a
> reader doesn't have to spend extra deciphering why 'connect2' tests
> appear before 'connect1' tests.
Please see attached v2 of the patch. It now includes same fix in
another TAP tests file.
Best regards,
Gurjeet https://Gurje.et
Postgres Contributors Team, http://aws.amazon.com
From 8e785d5b440f17f7788c722582b2613e092ca18c Mon Sep 17 00:00:00 2001
From: Gurjeet Singh
Date: Thu, 20 Apr 2023 22:49:00 -0700
Subject: [PATCH v2] Reorder connection markers in TAP tests
Commit 7f5b198 introduced TAP tests that use string literals to mark the
presence of a query in server logs. For no explicable reason, the tests
with the marker 'connect2' occur before the tests that use 'connect1'
marker.
So swap the connection marker strings so that a reader doesn't have to
spend extra effort deciphering why 'connect2' tests appear before
'connect1' tests.
---
.../libpq/t/003_load_balance_host_list.pl | 12 ++--
src/interfaces/libpq/t/004_load_balance_dns.pl | 14 +++---
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/interfaces/libpq/t/003_load_balance_host_list.pl b/src/interfaces/libpq/t/003_load_balance_host_list.pl
index 6963ef3849..7f1bb0c5bc 100644
--- a/src/interfaces/libpq/t/003_load_balance_host_list.pl
+++ b/src/interfaces/libpq/t/003_load_balance_host_list.pl
@@ -36,8 +36,8 @@ $node1->connect_fails(
# load_balance_hosts=disable should always choose the first one.
$node1->connect_ok("host=$hostlist port=$portlist load_balance_hosts=disable",
"load_balance_hosts=disable connects to the first node",
- sql => "SELECT 'connect2'",
- log_like => [qr/statement: SELECT 'connect2'/]);
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
# Statistically the following loop with load_balance_hosts=random will almost
# certainly connect at least once to each of the nodes. The chance of that not
@@ -45,12 +45,12 @@ $node1->connect_ok("host=$hostlist port=$portlist load_balance_hosts=disable",
foreach my $i (1 .. 50) {
$node1->connect_ok("host=$hostlist port=$portlist load_balance_hosts=random",
"repeated connections with random load balancing",
- sql => "SELECT 'connect1'");
+ sql => "SELECT 'connect2'");
}
-my $node1_occurences = () = $node1->log_content() =~ /statement: SELECT 'connect1'/g;
-my $node2_occurences = () = $node2->log_content() =~ /statement: SELECT 'connect1'/g;
-my $node3_occurences = () = $node3->log_content() =~ /statement: SELECT 'connect1'/g;
+my $node1_occurences = () = $node1->log_content() =~ /statement: SELECT 'connect2'/g;
+my $node2_occurences = () = $node2->log_content() =~ /statement: SELECT 'connect2'/g;
+my $node3_occurences = () = $node3->log_content() =~ /statement: SELECT 'connect2'/g;
my $total_occurences = $node1_occurences + $node2_occurences + $node3_occurences;
diff --git a/src/interfaces/libpq/t/004_load_balance_dns.pl b/src/interfaces/libpq/t/004_load_balance_dns.pl
index d9b382dba9..b07bc9c074 100644
--- a/src/interfaces/libpq/t/004_load_balance_dns.pl
+++ b/src/interfaces/libpq/t/004_load_balance_dns.pl
@@ -9,7 +9,7 @@ use Test::More;
if ($ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
{
plan skip_all =>
- 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+ 'Potentially unsafe test; load_balance not enabled in PG_TEST_EXTRA';
}
# This tests loadbalancing based on a DNS entry that contains multiple records
@@ -78,8 +78,8 @@ $node3->start();
# load_balance_hosts=disable should always choose the first one.
$node1->connect_ok("host=pg-loadbalancetest port=$port load_balance_hosts=disable",
"load_balance_hosts=disable connects to the first node",
- sql => "SELECT 'connect2'",
- log_like => [qr/statement: SELECT 'connect2'/]);
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
# Statistically the following loop with load_balance_hosts=random will almost
@@ -88,12 +88,12 @@ $node1->connect_ok("host=pg-loadbalancetest port=$port load_balance_hosts=disabl
foreach my $i (1 .. 50) {
$node1->connect_ok("host=pg-loadbalancetest port=$port load_balance_hosts=random",
"repeated connections with random load balancing",
- sql => "SELECT 'connect1'");
+ sql => "SELECT 'connect2'");
}
-my $node1_occurences = () = $node1->log_content() =~ /statement: SELECT 'connect1'/g;
-my $node2
Reorder connection markers in libpq TAP tests
Commit 7f5b198 introduced TAP tests that use string literals to mark the presence of a query in server logs. For no explicable reason, the tests with the marker 'connect2' occur before the tests that use 'connect1' marker. The attached patch swaps the connection marker strings so that a reader doesn't have to spend extra deciphering why 'connect2' tests appear before 'connect1' tests. Best regards, Gurjeet https://Gurje.et Postgres Contributors Team, http://aws.amazon.com v0-0001-Reorder-connection-markers-in-TAP-tests.patch Description: Binary data
Minor code de-duplication in fe-connect.c
Commit [1] implements Fisher-Yates shuffling algorithm to shuffle connection addresses, in two places. The attached patch moves the duplicated code to a function, and calls it in those 2 places. [1]: Support connection load balancing in libpq 7f5b19817eaf38e70ad1153db4e644ee9456853e Best regards, Gurjeet https://Gurje.et Postgres Contributors Team, http://aws.amazon.com v0-0001-Reduce-code-duplication-in-fe-connect.c.patch Description: Binary data
Re: Make message strings in fe-connect.c consistent
On Thu, Apr 20, 2023 at 9:31 PM Tom Lane wrote: > > Gurjeet Singh writes: > > When reviewing a recently committed patch [1] I noticed the odd usage > > of a format specifier: > > > + libpq_append_conn_error(conn, "invalid %s value: \"%s\"", > > + "load_balance_hosts", > > + conn->load_balance_hosts); > > > The oddity is that the first %s is unnecessary, since the value we > > want there is a constant. Typically a format specifier used to get the > > value stored in a variable. > > This is actually intentional, on the grounds that it reduces the > number of format strings that require translation. That's the only reason I too could come up with. > > There's just one exception to this pattern, though. > > >> libpq_append_conn_error(conn, "invalid require_auth method: \"%s\"", > >> method); > > Yup, this one did not get the memo. That explains why I could not find any translation for this error message. Best regards, Gurjeet http://Gurje.et Postgres Contributors Team, http://aws.amazon.com
Make message strings in fe-connect.c consistent
When reviewing a recently committed patch [1] I noticed the odd usage of a format specifier: + libpq_append_conn_error(conn, "invalid %s value: \"%s\"", + "load_balance_hosts", + conn->load_balance_hosts); The oddity is that the first %s is unnecessary, since the value we want there is a constant. Typically a format specifier used to get the value stored in a variable. Upon closer look, it looks like this is a common pattern in fe-connect.c; there are many places where a %s format specifier is being used in the format sting, where the name of the parameter would have sufficed. Upon some research, the only explanation I could come up with was that this pattern of specifying error messages helps with message translations. This way there's just one message to be translated. For example: .../libpq/po/es.po-#: fe-connect.c:1268 fe-connect.c:1294 fe-connect.c:1336 fe-connect.c:1345 .../libpq/po/es.po-#: fe-connect.c:1378 fe-connect.c:1422 .../libpq/po/es.po-#, c-format .../libpq/po/es.po:msgid "invalid %s value: \"%s\"\n" There's just one exception to this pattern, though. > libpq_append_conn_error(conn, "invalid require_auth method: \"%s\"", > method); So, to make it consistent throughout the file, we should either replace all such %s format specifiers with the respective strings, or use the same pattern for the message string used for require_method, as well. Attached patch [2] does the former, and patch [3] does the latter. Pick your favorite one. [1]: Support connection load balancing in libpq 7f5b19817eaf38e70ad1153db4e644ee9456853e [2]: Replace placeholders with known strings v1-0001-Replace-placeholders-with-known-strings.patch [3]: Make require_auth error message similar to surrounding messages v1-0001-Make-require_auth-error-message-similar-to-surrou.patch Best regards, Gurjeet http://Gurje.et Postgres Contributors Team, http://aws.amazon.com v1-0001-Replace-placeholders-with-known-strings.patch Description: Binary data v1-0001-Make-require_auth-error-message-similar-to-surrou.patch Description: Binary data
Re: Proposal: :SQL_EXEC_TIME (like :ROW_COUNT) Variable (psql)
On Thu, Feb 23, 2023 at 8:42 PM Kirk Wolak wrote: > I love that my proposal for %T in the prompt, triggered some great > conversations. > > This is not instead of that. That lets me run a query and come back HOURS > later, and know it finished before 7PM like it was supposed to! Neat! I have this info embedded in my Bash prompt [1], but many a times this is not sufficient to reconstruct the time it took to run the shell command. > This feature is simple. We forget to set \timing on... > We run a query, and we WONDER... how long did that take. And so I empathize with this need. I have set my Bash prompt to show me this info [2].This info is very helpful in situations where you fire a command, get tired of waiting for it and walk away for a few minutes. Upon return it's very useful to see exactly how long did it take for the command to finish. > I am not sure the name is right, but I would like to report it in the same > (ms) units as \timing, since there is an implicit relationship in what they > are doing. > > I think like ROW_COUNT, it should not change because of internal commands. +1 > So, you guys +1 this thing, give additional comments. When the feedback > settles, I commit to making it happen. This is definitely a useful feature. I agree with everything in the proposed UI (reporting in milliseconds, don't track internal commands' timing). I think 'duration' or 'elapsed' would be a better words in this context. So perhaps the name could be one of :sql_exec_duration (sql prefix feels superfluous), :exec_duration, :command_duration, or :elapsed_time. By using \timing, the user is explicitly opting into any overhead caused by time-keeping. With this feature, the timing info will be collected all the time. So do consider evaluating the performance impact this can cause on people's workloads. They may not care for the impact in interactive mode, but in automated scripts, even a moderate performance overhead would be a deal-breaker. [1]: https://github.com/gurjeet/home/blob/08f1051fb854f4fc8fbc4f1326f393ed507a55ce/.bashrc#L278 [2]: https://github.com/gurjeet/home/blob/08f1051fb854f4fc8fbc4f1326f393ed507a55ce/.bashrc#L262 Best regards, Gurjeet http://Gurje.et
Assert fcinfo has enough args before allowing parameter access (was: Re: generate_series for timestamptz and time zone problem)
On Mon, Jan 30, 2023 at 11:50 PM Gurjeet Singh wrote: > It was the classical case of out-of-bounds access. > This mistake would've been caught early if there were assertions > preventing access beyond the number of arguments passed to the > function. I'll send the assert_enough_args.patch, that adds these > checks, in a separate thread to avoid potentially confusing cfbot. Please see attached the patch to that ensures we don't accidentally access more parameters than that are passed to a SQL callable function. Best regards, Gurjeet http://Gurje.et diff --git a/src/include/fmgr.h b/src/include/fmgr.h index b120f5e7fe..a445ac56b9 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -206,7 +206,7 @@ extern void fmgr_symbol(Oid functionId, char **mod, char **fn); * If function is not marked "proisstrict" in pg_proc, it must check for * null arguments using this macro. Do not try to GETARG a null argument! */ -#define PG_ARGISNULL(n) (fcinfo->args[n].isnull) +#define PG_ARGISNULL(n) (AssertMacro(n < PG_NARGS()), fcinfo->args[n].isnull) /* * Support for fetching detoasted copies of toastable datatypes (all of @@ -265,7 +265,7 @@ extern struct varlena *pg_detoast_datum_packed(struct varlena *datum); /* Macros for fetching arguments of standard types */ -#define PG_GETARG_DATUM(n) (fcinfo->args[n].value) +#define PG_GETARG_DATUM(n) (AssertMacro(n < PG_NARGS()), fcinfo->args[n].value) #define PG_GETARG_INT32(n) DatumGetInt32(PG_GETARG_DATUM(n)) #define PG_GETARG_UINT32(n) DatumGetUInt32(PG_GETARG_DATUM(n)) #define PG_GETARG_INT16(n) DatumGetInt16(PG_GETARG_DATUM(n))
Re: generate_series for timestamptz and time zone problem
On Mon, Jan 30, 2023 at 4:07 PM Tom Lane wrote:
> Gurjeet Singh writes:
> > [ generate_series_with_timezone.v6.patch ]
>
> The cfbot isn't terribly happy with this. It looks like UBSan
> is detecting some undefined behavior. Possibly an uninitialized
> variable?
It was the classical case of out-of-bounds access. I was trying to
access 4th argument, even in the case where the 3-argument variant of
generate_series() was called.
Please see attached v7 of the patch. It now checks PG_NARGS() before
accessing the optional parameter.
This mistake would've been caught early if there were assertions
preventing access beyond the number of arguments passed to the
function. I'll send the assert_enough_args.patch, that adds these
checks, in a separate thread to avoid potentially confusing cfbot.
Best regards,
Gurjeet
http://Gurje.et
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e09e289a43..aa15407936 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -9231,6 +9231,22 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
+
+
+ date_add ( timestamp with time zone, interval , text )
+ timestamp with time zone
+
+
+ Add interval to a timestamp with time zone value,
+ at the time zone specified by the third parameter. The time zone value
+ defaults to current setting.
+
+
+ date_add('2021-10-31 00:00:00+02'::timestamptz, '1 day'::interval, 'Europe/Warsaw')
+ 2021-10-31 23:00:00
+
+
+
date_bin ( interval, timestamp, timestamp )
@@ -9278,6 +9294,22 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
+
+
+ date_subtract ( timestamp with time zone, interval , text )
+ timestamp with time zone
+
+
+ Subtract interval from a timestamp with time zone value,
+ at the time zone specified by the third parameter. The time zone value
+ defaults to the current setting.
+
+
+ date_subtract('2021-10-31 00:00:00+02'::timestamptz, '1 day'::interval, 'Europe/Warsaw')
+ 2021-10-29 22:00:00
+
+
+
@@ -21968,13 +22000,14 @@ AND
setof timestamp
-generate_series ( start timestamp with time zone, stop timestamp with time zone, step interval )
+generate_series ( start timestamp with time zone, stop timestamp with time zone, step interval , timezone text )
setof timestamp with time zone
Generates a series of values from start
to stop, with a step size
-of step.
+of step. timezone
+defaults to the current setting.
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 47e059a409..bd85f6421e 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -69,6 +69,7 @@ typedef struct
TimestampTz finish;
Interval step;
int step_sign;
+ pg_tz *attimezone;
} generate_series_timestamptz_fctx;
@@ -78,6 +79,8 @@ static bool AdjustIntervalForTypmod(Interval *interval, int32 typmod,
Node *escontext);
static TimestampTz timestamp2timestamptz(Timestamp timestamp);
static Timestamp timestamptz2timestamp(TimestampTz timestamp);
+static pg_tz* lookup_timezone(text *zone);
+static Datum generate_series_timestamptz_internal(FunctionCallInfo fcinfo);
/* common code for timestamptypmodin and timestamptztypmodin */
@@ -550,6 +553,54 @@ parse_sane_timezone(struct pg_tm *tm, text *zone)
return tz;
}
+/*
+ * Look up the requested timezone (see notes in timestamptz_zone()).
+ */
+static pg_tz *
+lookup_timezone(text *zone)
+{
+ char tzname[TZ_STRLEN_MAX + 1];
+ char *lowzone;
+ int type,
+dterr,
+val;
+ pg_tz *tzp;
+
+ DateTimeErrorExtra extra;
+
+ text_to_cstring_buffer(zone, tzname, sizeof(tzname));
+
+ /* DecodeTimezoneAbbrev requires lowercase input */
+ lowzone = downcase_truncate_identifier(tzname,
+ strlen(tzname),
+ false);
+
+ dterr = DecodeTimezoneAbbrev(0, lowzone, , , , );
+ if (dterr)
+ DateTimeParseError(dterr, , NULL, NULL, NULL);
+
+ if (type == TZ || type == DTZ)
+ {
+ /* fixed-offset abbreviation, get a pg_tz descriptor for that */
+ tzp = pg_tzset_offset(-val);
+ }
+ else if (type == DYNTZ)
+ {
+ /* dynamic-offset abbreviation, use its referenced timezone */
+ }
+ else
+ {
+ /* try it as a full zone name */
+ tzp = pg_tzset(tzname);
+ if (!tzp)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("time zone \"%s\" not recognized", tzname)));
+ }
+
+ return tzp;
+}
+
/*
* make_timestamp_internal
* workhorse for make_timestamp and make_timestamptz
@@ -3014,97 +3065,112
Re: generate_series for timestamptz and time zone problem
On Sat, Nov 12, 2022 at 10:44 AM Tom Lane wrote: > However, what it shouldn't be is part of this patch. It's worth > pushing it separately to have a record of that decision. I've > now done that, so you'll need to rebase to remove that delta. > > I looked over the v5 patch very briefly, and have two main > complaints: > > * There's no documentation additions. You can't add a user-visible > function without adding an appropriate entry to func.sgml. > > * I'm pretty unimpressed with the whole truncate-to-interval thing > and would recommend you drop it. I don't think it's adding much > useful functionality beyond what we can already do with the existing > date_trunc variants; and the definition seems excessively messy > (too many restrictions and special cases). Please see attached v6 of the patch. The changes since v5 are: .) Rebased and resolved conflicts caused by commit 533e02e92. .) Removed code and tests related to new date_trunc() functions, as suggested by Tom. .) Added 3 more variants to accompany with date_add(tstz, interval, zone). date_add(tstz, interval) date_subtract(tstz, interval) date_subtract(tstz, interval, zone) .) Eliminate duplication of code; use common function to implement generate_series_timestamptz[_at_zone]() functions. .) Fixed bug where in one of the new code paths, generate_series_timestamptz_with_zone(), did not perform TIMESTAMP_NOT_FINITE() check. .) Replaced some DirectFunctionCall?() with direct calls to the relevant *_internal() function; should be better for performance. .) Added documentation all 5 functions (2 date_add(), 2 date_subtract(), 1 overloaded version of generate_series()). I'm not sure of the convention around authorship. But since this was not an insignificant amount of work, would this patch be considered as co-authored by Przemyslaw and myself? Should I add myself to Authors field in the Commitfest app? Hi Przemyslaw, I started working on this patch based on Tom's review a few days ago, since you hadn't responded in a while, and I presumed you're not working on this anymore. I should've consulted with/notified you of my intent before starting to work on it, to avoid duplication of work. Sorry if this submission obviates any work you have in progress. Please feel free to provide your feedback on the v6 of the patch. Best regards, Gurjeet http://Gurje.et generate_series_with_timezone.v6.patch Description: Binary data
Re: Named Operators
On Fri, Jan 20, 2023 at 9:32 AM Ted Yu wrote: > > Since `validIdentifier` doesn't modify the contents of `name` string, it > seems that there is no need to create `tmp` string in `validNamedOperator`. > You can pass the start and end offsets into the string (name) as second and > third parameters to `validIdentifier`. Thanks for reviewing the patch! I was making a temporary copy of the string, since I had to modify it before the validation, whereas the callee expects a `const char*`. I agree that the same check can be done with more elegance, while eliminating the temporary allocation. Please find the updated patch attached. Instead of passing the start and end of region I want to check, as suggested, I'm now passing just the length of the string I want validated. But I think that's for the better, since it now aligns with the comment that validIdentifier() does not check if the passed string is shorter than NAMEDATALEN. Best regards, Gurjeet http://Gurje.et named_operators_v4.patch Description: Binary data
Re: Named Operators
On Sat, Jan 14, 2023 at 6:14 AM Gurjeet Singh wrote:
>
> I agree that an identifier _surrounded_ by the same token (e.g. #foo#)
> or the pairing token (e.g. {foo}) looks better aesthetically, so I am
> okay with any of the following variations of the scheme, as well:
>
> \#foo\# (tested; works)
> \#foo# (not tested; reduces ident length by 1)
>
> We can choose a different character, instead of #. Perhaps \{foo} !
Please find attached the patch that uses \{foo} styled Named
Operators. This is in line with Tom's reluctant hint at possibly using
curly braces as delimiter characters. Since the curly braces are used
by the SQL Specification for row pattern recognition, this patch
proposes escaping the first of the curly braces.
We can get rid of the leading backslash, if (a) we're confident that
SQL committee will not use curly braces anywhere else, and (b) if
we're confident that if/when Postgres supports Row Pattern Recognition
feature, we'll be able to treat curly braces inside the PATTERN clause
specially. Since both of those conditions are unlikely, I think we
must settle for the escaped-first-curly-brace style for the naming our
operators.
Keeping with the previous posts, here's a sample SQL script showing
what the proposed syntax will look like in action. Personally, I
prefer the \#foo style, since the \# prefix stands out among the text,
better than \{..} does, and because # character is a better signal of
an operator than {.
create operator \{add_point}
(function = box_add, leftarg = box, rightarg = point);
create table test(a box);
insert into test values('((0,0),(1,1))'), ('((0,0),(2,1))');
select a as original, a \{add_point} '(1,1)' as modified from test;
drop operator \{add_point}(box, point);
Best regards,
Gurjeet
http://Gurje.et
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 1017f2eed1..c5b8562cb5 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -31,6 +31,7 @@
#include "catalog/pg_type.h"
#include "miscadmin.h"
#include "parser/parse_oper.h"
+#include "parser/scansup.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
@@ -79,6 +80,10 @@ validOperatorName(const char *name)
if (len == 0 || len >= NAMEDATALEN)
return false;
+ /* Is this a Named Operator? */
+ if (validNamedOperator(name))
+ return true;
+
/* Can't contain any invalid characters */
/* Test string here should match op_chars in scan.l */
if (strspn(name, "~!@#^&|`?+-*/%<>=") != len)
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index db8b0fe8eb..8587b82c8d 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -379,6 +379,16 @@ self [,()\[\].;\:\+\-\*\/\%\^\<\>\=]
op_chars [\~\!\@\#\^\&\|\`\?\+\-\*\/\%\<\>\=]
operator {op_chars}+
+/*
+ * Named Operators, e.g. \{foo}
+ *
+ * {namedopfailed*} are error rules to avoid scanner backup when
+ * {namedop} fails to match its trailing tokens.
+ */
+namedop \\\{{identifier}\}
+namedopfailed1 \\\{{identifier}
+namedopfailed2 \\\{
+
/*
* Numbers
*
@@ -768,6 +778,23 @@ other .
}
<> { yyerror("unterminated dollar-quoted string"); }
+{namedop} {
+ SET_YYLLOC();
+ if (yyleng >= NAMEDATALEN)
+ yyerror("operator name too long");
+ /* XXX Should we support double-quoted, case sensitive names? */
+ yylval->str = downcase_identifier(yytext, yyleng, false, false);
+ return Op;
+}
+
+{namedopfailed1} {
+ yyerror("unexpected token");
+}
+
+{namedopfailed2} {
+ yyerror("unexpected token");
+}
+
{xdstart} {
SET_YYLLOC();
BEGIN(xd);
diff --git a/src/backend/parser/scansup.c b/src/backend/parser/scansup.c
index 602108a40f..05c46ae09e 100644
--- a/src/backend/parser/scansup.c
+++ b/src/backend/parser/scansup.c
@@ -125,3 +125,70 @@ scanner_isspace(char ch)
return true;
return false;
}
+
+/*
+ * validNamedOperator() -- return true if name adheres to the scanner rule
+ * {namedop}
+ */
+bool
+validNamedOperator(const char *name)
+{
+ size_t len = strlen(name);
+ bool valid_identifier;
+ char *tmp;
+
+ if (len < 4 || len >= NAMEDATALEN)
+ return false;
+
+ if (name[0] != '\\' || name[1] != '{' || name[len-1] != '}')
+ return false;
+
+ tmp = pstrdup(name);
+
+ // Disregard the delimiters
+ tmp[len-1] = '\0';
+ valid_identifier = validIdentifier(tmp + 2);
+ pfree(tmp);
+
+ return valid_identifier;
+}
+
+/*
+ * validIdentifier() -- return true if name adheres to the scanner rule
+ * {identifier}
+ *
+ * Note: this function does not check if the identifier length
+ * is less than NAMEDATALEN.
+ */
+bool
+validIdentifier(const char *name)
+{
+ uint8 c;
+ size_t i, len = strlen(name);
+
+ // Reject if first character is not part of ident_start
Re: Named Operators
On Thu, Jan 12, 2023 at 5:55 AM Matthias van de Meent
wrote:
> On Thu, 12 Jan 2023 at 11:59, Gurjeet Singh wrote:
> > ... defining an operator
> > gives you many additional features that the planner can use to
> > optimize your query differently, which it can't do with functions. See
> > the COMMUTATOR, HASHES, etc. clause in the CREATE OPERATOR command.
>
> I see. Wouldn't it be better then to instead make it possible for the
> planner to detect the use of the functions used in operators and treat
> them as aliases of the operator? Or am I missing something w.r.t.
> differences between operator and function invocation?
>
> E.g. indexes on `int8pl(my_bigint, 1)` does not match queries for
> `my_bigint + 1` (and vice versa), while they should be able to support
> that, as OPERATOR(pg_catalog.+(int8, int8)) 's function is int8pl.
Such a feature would be immensely useful in its own right. But it's
also going to be at least 2 orders of magnitude (or more) effort to
implement, and to get accepted in the community. I'm thinking of
changes in planner, catalogs, etc.
On Thu, Jan 12, 2023 at 7:21 AM Tom Lane wrote:
> Matthias van de Meent writes:
> > I'm -1 on the chosen syntax; :name: shadows common variable
> > substitution patterns including those of psql.
>
> Yeah, this syntax is DOA because of that. I think almost
> anything you might invent is going to have conflict risks.
I remember discussing this in a meeting with Joe Conway a few weeks
ago, when this was just a proposal in my head and I was just bouncing
it off him. And I remember pointing out that colons would be a bad
choice because of their use in psql; but for life of me I can't think
of a reason (except temporary memory loss) why I failed to consider
the psql conflict when implementing the feature. If only some test in
`make check` would have pointed out the mistake, I wouldn't have made
this obvious mistake.
> We could probably make it work by allowing the existing OPERATOR
> syntax to take things that look like names as well as operators,
> like
>
> expr3 OPERATOR(contains_all) expr4
>
> But that's bulky enough that nobody will care to use it.
+1. Although that'd be better for readers than the all-special-char
names, this format is bulky enough that you won't be able to convince
the query writers to bother using it. But if all other efforts fail,
I'll take this format over the cryptic ones any day.
> On the whole I don't see this proposal going anywhere.
> There's too much investment in the existing operator names,
> and too much risk of conflicts if you try to shorten the
> syntax.
I wouldn't give up on the idea, yet :-) See new proposal below.
On Thu, Jan 12, 2023 at 9:14 AM Tom Lane wrote:
> Isaac Morland writes:
> > What about backticks (`)?
>
> Since they're already allowed as operator characters, you can't
> use them for this purpose without breaking existing use-cases.
>
> Even if they were completely unused, I'd be pretty hesitant to
> adopt them for this purpose because of the potential confusion
> for users coming from mysql.
Since when have we started caring for the convenience of users of
other databases?!! /s
> Pretty much the only available syntax space is curly braces,
> and I don't really want to give those up for this either.
> (One has to assume that the SQL committee has their eyes
> on those too.)
On Thu, Jan 12, 2023 at 9:45 AM Vik Fearing wrote:
> They are used in row pattern recognition.
I was very hopeful of using { }, and hoping that we'd beat the SQL
committee to it, so that they have to choose something else, if we
release this into the wild before them. But it seems that they beat us
to it long ago. (tangent: Reading some blog posts, I have to say I
loved the Row Pattern Recognition feature!)
Considering that there are almost no printable characters left in
1-255 ASCII range for us to choose from, I had to get creative; and I
believe I have found a way to make it work.
Unless the SQL committee has their eyes on a freestanding backslash \
character for something, I believe we can use it as a prefix for Named
Operators. Since the most common use of backslash is for escaping
characters, I believe it would feel natural for the users to use it as
described below.
New scheme for the named operators: \#foo That is, an identifier
prefixed with \# would serve as an operator name. psql considers \ to
be the start of its commands, but it wasn't hard to convince psql to
ignore \# and let it pass through to server.
I agree that an identifier _surrounded_ by the same token (e.g. #foo#)
or the pairing token (e.g. {foo}) looks better aesthetically, so I am
okay with any of the following variations of the scheme, as well:
\#foo\# (tested; works)
\#foo# (not tested; reduces ident length by 1)
We can choose a different character, instead of #.
Re: Named Operators
On Thu, Jan 12, 2023 at 1:49 AM Matthias van de Meent wrote: > > On Thu, 12 Jan 2023 at 10:16, Gurjeet Singh wrote: > > > > Technically correct name of this feature would be Readable Names for > > Operators, or Pronounceable Names for Operators. But I'd like to call > > it Named Operators. > > > > With this patch in place, the users can name the operators as > > :some_pronounceable_name: instead of having to choose from the special > > characters like #^&@. > > [...] > > I think Named Operators will significantly improve the readability > > of queries. > > Couldn't the user better opt to call the functions that implement the > operator directly if they want more legible operations? So, from your > example, `SELECT box_add(a, b)` instead of `SELECT a :add_point: b`? Matter of taste, I guess. But more importantly, defining an operator gives you many additional features that the planner can use to optimize your query differently, which it can't do with functions. See the COMMUTATOR, HASHES, etc. clause in the CREATE OPERATOR command. https://www.postgresql.org/docs/current/sql-createoperator.html This proposal is primarily a replacement for the myriad of hard-to-pronounce operators that users have to memorize. For example, it'd be nice to have readable names for the PostGIS operators. https://postgis.net/docs/reference.html#Operators For someone who's reading/troubleshooting a PostGIS query, when they encounter operator <<| — in the query for the first time, they'd have to open up the docs. But if the query used the :strictly_below: operator, there's no need to switch to docs and lose context. > I'm -1 on the chosen syntax; :name: shadows common variable > substitution patterns including those of psql. Ah, thanks for reminding! Early on when I hadn't written code yet, I remember discarding colon : as a delimiter choice, precisely because it is used for using variables in psql, and perhaps in some drivers, as well. But in the rush of implementing and wrangling code, I forgot about that argument altogether. I'll consider using one of the other special characters. Do you have any suggestions? Best regards, Gurjeet http://Gurje.et
Re: Named Operators
Please see attached a slightly updated patch. There were some comment changes sitting in uncommitted in Git worktree, that were missed. Best regards, Gurjeet http://Gurje.et named_operators_v1.patch Description: Binary data
Named Operators
Technically correct name of this feature would be Readable Names for
Operators, or Pronounceable Names for Operators. But I'd like to call
it Named Operators.
With this patch in place, the users can name the operators as
:some_pronounceable_name: instead of having to choose from the special
characters like #^&@. For example, users will be able to create and
use operators like:
select
expr1 :distance: expr2,
expr3 :contains_all: expr4,
expr5 :contains_any: expr6
expr7 :contains_exactly_two_of: expr8
from mytable;
instead of being forced to use these:
select
expr1 <#> expr2,
expr3 ?& expr4,
expr5 ?| expr6
expr7 ??!! expr8 -- ¯\_(ツ)_/¯
from mytable;
I think Named Operators will significantly improve the readability
of queries.
After a little trial-an-error, it was easy to develop the scan.l
rules to implement this feature, without flex barfing. The hard part
has been convincing myself that this is a safe implementation, even
though there are no regressions in `make check`. I am unsure of this
implementation's compatibility with the SQL Spec, and I'm not able to
envision problems with its interaction with some current or potential
feature of Postgres. So I'd really appreciate feedback from someone
who is conversant with the SQL Spec.
If the colon character being used as a delimiter poses a
challenge, other good candidates for the delimiter seem to be one of
~^` Although I haven't tested any of these to see if they cause a
regression. The colon character is be preferable for the delimiter,
since it is already used in the typecast :: operator.
I tried to strip the delimiters/colons from the name right in the
scanner, primarily because that would allow the identifier part of the
name to be as long as NAMEDATALEN-1, just like other identifiers
Postgres allows. Added benefit of stripping delimiters was that the
rest of the code, and catalogs/storage won't have to see the
delimiters. But stripping the delimiters made the code brittle; some
places in code now had to be taught different handling depending on
whether the operator name was coming from the user command, or from
the catalogs. I had to special-case code in pg_dump, as well. To share
code with frontends like pg_dump, I had to place code in src/common/.
I was still not able to address some obvious bugs.
By retaining the delimiters : in the name, the code became much
simpler; pg_dump support came for free! The bugs became a non-issue.
To see how much code and complexity was reduced, one can see this
commit [1]. The downside of retaining the delimiters is that the
identifier part of the name can be no more than NAMEDATALEN-3 in
length.
Because of the minimal changes to the scanner rules, and no
changes in the grammar, I don't think there's any impact on precedence
and associativity rules of the operators. I'd be happy to learn
otherwise.
Here's a rudimentary test case to demonstrate the feature:
create operator :add_point: (function = box_add, leftarg = box,
rightarg = point);
create table test(a box);
insert into test values('((0,0),(1,1))'), ('((0,0),(2,1))');
select a as original, a :add_point: '(1,1)' as modified from test;
drop operator :add_point:(box, point);
Feedback will be much appreciated!
[1]: Commit: Don't strip the delimiters
https://github.com/gurjeet/postgres/commit/62d11a578e5325c32109bb2a55a624d0d89d4b7e
[2]: Git branch named_operators
https://github.com/gurjeet/postgres/tree/named_operators
Best regards,
Gurjeet
http://Gurje.et
named_operators.patch
Description: Binary data
Re: pg_reload_conf() synchronously
On Sat, Nov 5, 2022 at 11:23 AM Andres Freund wrote: > > As far as I know, Gurjeet has been annoyed only with non-user-settable > > GUCs for one connection (correct me of course), there was nothing > > fancy with isolation tests, yet. Not saying that this is useless for > > isolation tests, this would have its cases for assumptions where > > multiple GUCs need to be synced across multiple sessions, but it seems > > to me that we have two different cases in need of two different > > solutions. > > I didn't say we need to go for something more complete. Just that it's worth > thinking about. FWIW, I have considered a few different approaches, but all of them were not only more work, they were fragile, and diffcult to prove correctness of. For example, I thought of implementing DSM based synchronisation between processes, so that the invoking backend can be sure that _all_ children of Postmaster have processed the reload. But that will run into problems as different backends get created, and as they exit. The invoking client may be interested in just client-facing backends honoring the reload before moving on, so it would have to wait until all the other backends finish their current command and return to the main loop; but that may never happen, because one of the backends may be processing a long-running query. Or, for some reason, the the caller may be interested in only the autovacuum proccesses honoring its reload request. So I thought about creating _classes_ of backends: client-facing, other always-on children of Postmaster, BGWorkers, etc. But that just makes the problem more difficult to solve. I hadn't considered the possibilty of deadlocks that Tom raised, so that's another downside of the other approaches. The proposed patch solves a specific problem, that of a single backend reloading conf before the next command, without adding any complexity for any other cases. It sure is worth solving the case where a backend waits for another backed(s) to process the conf files, but it will have to be a separate solution, becuase it's much more difficult to get it right. Best regards, Gurjeet http://Gurje.et
pg_reload_conf() synchronously
In an internal conversation it was seen that for some tests that want to enforce a behaviour, a behaviour that is controlled by a GUC, we _need_ to perform a sleep for an arbitrary amount of time. Alternatively, executing the rest of the test on a new connection also helps to get the expected behaviour. Following is a sample snippet of such a test. ALTER SYSTEM SET param TO value; SELECT pg_reload_conf(); -- Either pg_sleep(0.1) or \connect here for next command to behave as expected. ALTER ROLE ... PASSWORD '...'; This is because of the fact that the SIGHUP, sent to Postmaster by this backend, takes some time to get back to the issuing backend. Although a pg_sleep() call works to alleviate the pain in most cases, it does not provide any certainty. Following the Principle Of Least Astonishment, we want a command that loads the configuration _synchronously_, so that the subsequent commands from the client can be confident that the requisite parameter changes have taken effect. The attached patch makes the pg_reload_conf() function set ConfigReloadPending to true, which will force the postgres main command-processing loop to process and apply config changes _before_ executing the next command. The only downside I can think of this approach is that it _might_ cause the issuing backend to process the config file twice; once after it has processed the current command, and another time when the SIGHUP signal comes from the Postmaster. If the SIGHUP signal from the Postmaster comes before the end of current command, then the issuing backend will process the config only once, as before the patch. In any case, this extra processing of the config seems harmless, and the benefit outweighs the extra processing done. The alternate methods that I considered (see branch reload_my_conf at [2]) were to either implement the SQL command RELOAD CONFIGURATION [ FOR ALL ], or to create an overloaded version of function pg_reload_conf(). But both those approaches had much more significant downsides, like additional GRANTs, etc. There have been issues identified in the past (see [1]) that possibly may be alleviated by this approach of applying config changes synchronously. [1]: https://www.postgresql.org/message-id/2138662.1623460441%40sss.pgh.pa.us [2]: https://github.com/gurjeet/postgres/tree/reload_my_conf Best regards, Gurjeet http://Gurje.et reload_conf_synchronouly.patch Description: Binary data
pg_regress: lookup shellprog in $PATH
I'm trying to build Postgres using the Nix language and the Nix package manager on macOS (see [1]). After some work I was able to build, and even run Postgres. But `make check` failed with the error pg_regress: could not exec "sh": No such file or directory The reason is that pg_regress uses execl() function to execute the shell recorded in its shellprog variable, whose value is provided using the SHELL variable via the makefile. Specifically, this error is because execl() function expects the full path of the executable as the first parameter, and it does _not_ perform a lookup in $PATH. Using execl() in pg_regress has worked in the past, because the default value of SHELL used by GNUmake (and I presume other make implementations) is /bin/sh, and /bin/sh expected to be present on any Unix-like system. But in Nixpkgs (the default and the central package repository of Nix/NixOS distro), they have chosen to patch GNU make, and turn the default value of SHELL from '/bin/sh' to just 'sh'; see their patch [2]. They did this because Nixpkgs consider any files outside the Nix Store (the path /nix/store/, by default) to be "impure". They want the packagers to use $PATH (consisting solely of paths that begin with /nix/store/...), to lookup their binaries and other files. So when pg_regress tries to run a program (the postmaster, in this case), the execl() function complains that it could not find 'sh', since there's no file ./sh in the directory where pg_regress is being run. Please see attached the one-letter patch that fixes this problem. I have chosen to replace the execl() call with execlp(), which performs a lookup in $PATH, and finds the 'sh' to use for running the postmaster. This patch does _not_ cause 'make check' or any other failures when Postgres is built with non-Nix build tools available on macOS. There is one other use of execl(), in pg_ctl.c, but that is safe from the behaviour introduced by Nixpkgs, since that call site uses the absolute path /bin/sh, and hence there's no ambiguity in where to look for the executable. There are no previous uses of execlp() in Postgres, which made me rethink this patch. But I think it's safe to use execlp() since it's part of POSIX, and it's also supported by Windows (see [3]; they say the function name is "deprecated" but function is "supported" in the same paragraph!!). There's one mention of execl in src/pl/plperl/ppport.h, and since it's a generated file, I believe now execlp also needs to be added to that list. But I'm not sure how to generate that file, or if it really needs to be generated and included in this patch; maybe the file is re-generated during a release process. Need advice on that. GNU make's docs clearly explain (see [4]) the special handling of variable SHELL, and it seems impossible to pass this variable from an env variable down to the GNUmakefile of interest. The only alternative I see for us being able to pass a custom value via SHELL, is to detect and declare the SHELL variable in one of our higher-level files; and I don't think that'd be a good idea. We could propose to Nixpkgs community that they stop patching make, and leave the default SHELL value alone. But I see very low likelihood of them accepting our arguments, or changing their ways. It took many days of debugging, troubleshooting etc, to get to this root-cause. I first tried to coax autoconf, make, etc. to pass my custom SHELL through to pg_regress' makefile. Changing CONFIG_SHELL, or SHELL does not have any impact. Then I had to read the Nixpkgs code, and work out the archaic ways the packages are defined, and after much code wrangling I was able to find out that _they_ changed the default value of SHELL by patching the make sources. The Nix language is not so bad, but the way it's used to write code in the Nix community leaves a lot to be desired; ad-hoc environment variable naming, polluting the built environment with all kinds of variables, almost non-existent comments, no standards on indentation, etc. These reasons made me decide to use the plain Nix language as much as possible, and not rely on Nixpkgs, whenever I can avoid it. The Nixpkgs and NixOS distro includes all the supported versions of Postgres, so one would assume they would've also encountered, and solved, this problem. But they didn't. My best guess as to why, is, I believe they never bothered to run `make check` on their built binaries. [1]: https://github.com/DrPostgres/HaathiFarm/blob/master/default.nix [2]: https://github.com/NixOS/nixpkgs/blob/release-22.05/pkgs/development/tools/build-managers/gnumake/0001-No-impure-bin-sh.patch [3]: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/execlp?view=msvc-170 [4]: https://www.gnu.org/software/make/manual/html_node/Choosing-the-Shell.html Best regards, Gurjeet http://Gurje.et pg_regress_use_execlp_to_lookup_shellprog.patch Description: Binary data
Re: Patch proposal: New hooks in the connection path
On Tue, Aug 16, 2022 at 3:16 AM Bharath Rupireddy
wrote:
>
> On Tue, Aug 16, 2022 at 1:55 PM Drouvot, Bertrand wrote:
> >
> > Hi,
> >
> > On 8/16/22 10:10 AM, Bharath Rupireddy wrote:
> > > On Tue, Aug 16, 2022 at 1:31 PM Drouvot, Bertrand
> > > wrote:
> > >> On 8/14/22 7:52 AM, Gurjeet Singh wrote:
> > >>> On Mon, Aug 8, 2022 at 3:51 AM Drouvot, Bertrand
> > >>> wrote:
> > >>> I think we can reduce the number of places the hook is called, if we
> > >>> call the hook from proc_exit(), and at all the other places we simply
> > >>> set
> > >>> a global variable to signify the reason for the failure. The case of
> > >>> _exit(1) from the signal-handler cannot use such a mechanism, but I
> > >>> think all the other cases of interest can simply register one of the
> > >>> FCET_* values, and let the call from proc_exit() pass that value
> > >>> to the hook.
> > >> That looks like a good idea to me. I'm tempted to rewrite the patch that
> > >> way (and addressing the first comment in the same time).
> > >>
> > >> Curious to hear about others hackers thoughts too.
I agree that we need feedback from long-timers here, on the decision
of whether to use proc_exit() for this purpose.
> > > IMO, calling the hook from proc_exit() is not a good design as
> > > proc_exit() is a generic code called from many places in the source
> > > code, even the simple code of kind if(call_failed_conn_hook) {
> > > falied_conn_hook(params);} can come in the way of many exit code paths
> > > which is undesirable, and the likelihood of introducing new bugs may
> > > increase.
> >
> > Thanks for the feedback.
> >
> > What do you think about calling the hook only if the new global variable
> > is not equal to its default value (which would mean don't trigger the
> > hook)?
>
> IMO, that's not a good design as explained above. Why should the
> failed connection hook related code get hit for each and every
> proc_exit() call? Here, the code duplication i.e. the number of places
> the failed connection hook gets called mustn't be the reason to move
> that code to proc_exit().
I agree, it doesn't feel _clean_, having to maintain a global
variable, pass it to hook at exit, etc. But the alternative feels less
cleaner.
This hook needs to be called when the process has decided to exit, so
it makes sense to place this call in stack above proc_exit(), whose
sole job is to let the process die gracefully, and take care of things
on the way out.
There are quite a few places in core that leverage proc_exit()'s
facilities (by registering on_proc_exit callbacks), so an
extension/hook doing so wouldn't be out of the ordinary; (apparently
contrib/sepgsql has already set the precedent on an extension using
the on_proc_exit callback). Admittedly, in this case the core will be
managing and passing it the additional global variable needed to
record the connection failure reason, FCET_*.
If we agree that proc_exit() is a good place to place this call, then
this hook can be converted into a on_proc_exit callback. If the global
variable is exported, then the extension(s) can access it in the
callback to ascertain why the process is exiting, and proc_exit()
won't have to know anything special about the extension, or hook, or
the global variable.
The on_proc_exit callback method wouldn't work for the _exit() called
in StartupPacketTimeoutHandler(), so that will need to be handled
separately.
Best regards,
Gurjeet
http://Gurje.et
Re: Patch proposal: New hooks in the connection path
(reposting the same review, with many grammatical fixes)
On Mon, Aug 8, 2022 at 3:51 AM Drouvot, Bertrand wrote:
> Please find attached v2-0004-connection_hooks.patch
/*
* Stop here if it was bad or a cancel packet. ProcessStartupPacket
* already did any appropriate error reporting.
*/
if (status != STATUS_OK)
+{
+#ifndef EXEC_BACKEND
+if (FailedConnection_hook)
+(*FailedConnection_hook) (FCET_BAD_STARTUP_PACKET, port);
+#endif
proc_exit(0);
+}
Per the comment above the if condition, the `status != OK` may
represent a cancel packet, as well. Clearly, a cancel packet is not
the same as a _bad_ packet. So I think here you need to differentiate
between a cancel packet and a genuinely bad packet; I don't see
anything good coming out of us, or the hook-developer, lumping
those 2 cases together.
I think we can reduce the number of places the hook is called, if we
call the hook from proc_exit(), and at all the other places we simply set
a global variable to signify the reason for the failure. The case of
_exit(1) from the signal-handler cannot use such a mechanism, but I
think all the other cases of interest can simply register one of the
FCET_* values, and let the call from proc_exit() pass that value
to the hook.
If we can convince ourselves that we can use proc_exit(1) in
StartupPacketTimeoutHandler(), instead of calling _exit(1), I think we
cal replace all call sites for this hook with the
set-global-variable variant.
> ...
> * This should be the only function to call exit().
> * -cim 2/6/90
>...
> proc_exit(int code)
The comment on proc_exit() claims that it should be the only place
calling exit(), except that the add-on/extension hooks may ignore this.
So there must be a strong reason why the signal-handler uses _exit()
to bypass all callbacks.
Best regards,
Gurjeet
http://Gurje.et
Re: Patch proposal: New hooks in the connection path
On Mon, Aug 8, 2022 at 3:51 AM Drouvot, Bertrand wrote:
> Please find attached v2-0004-connection_hooks.patch
/*
* Stop here if it was bad or a cancel packet. ProcessStartupPacket
* already did any appropriate error reporting.
*/
if (status != STATUS_OK)
+{
+#ifndef EXEC_BACKEND
+if (FailedConnection_hook)
+(*FailedConnection_hook) (FCET_BAD_STARTUP_PACKET, port);
+#endif
proc_exit(0);
+}
Per the comment above the if condition, the `status != OK` may
represent a cancel packet, as well. Clearly, a cancel packet is not
the same as a _bad_ packet. So I think here you need to differentiate
between a cancel packet and a genuinely bad packet; I don't see
anything good coming good out of us, or the hook-developer, lumping
those 2 cases together.
I think we can reduce the number of places the hook is called, if we
call the hook from proc_exit(), and all the other places we simply set
a global variable to signify the reason for the failure. The case of
_exit(1) from the signal-handler cannot use such a mechanism, but I
think all the other cases of interest can simply register one of the
FCET_* value, and the hook call from proc_exit() can pass that value
to the hook.
If we can convinces ourselves that we can use proc_exit(1) in
StartupPacketTimeoutHandler(), instead of calling _exit(1), I think we
cal eliminate replace all call sites for this hook with
set-global-variable variant.
> ...
> * This should be the only function to call exit().
> * -cim 2/6/90
>...
> proc_exit(int code)
The comment on proc_exit() claims that should be the only place
calling exit(), except the add-on/extension hooks. So there must be a
strong reason why the signal-handler uses _exit() to bypass all
callbacks.
Best regards,
Gurjeet
http://Gurje.et
Re: `make check` doesn't pass on MacOS Catalina
On Tue, Apr 20, 2021 at 9:06 AM Andrew Dunstan wrote: > > On 4/20/21 11:02 AM, Tom Lane wrote: > > Aleksander Alekseev writes: > >> While trying to build PostgreSQL from source (master branch, 95c3a195) on a > >> MacBook I discovered that `make check` fails: > > This is the usual symptom of not having disabled SIP :-(. > > > > If you don't want to do that, do "make install" before "make check". > FYI the buildfarm client has a '--delay-check' option that does exactly > this. It's useful on Alpine Linux as well as MacOS I was trying to set up a buildfarm animal, and this exact problem lead to a few hours of debugging and hair-pulling. Can the default behaviour be changed in buildfarm client to perform `make check` only after `make install`. Current buildfarm client code looks something like: make(); make_check() unless $delay_check; ... other steps ... make_install(); ... other steps-2... make_check() if $delay_check; There are no comments as to why one should choose to use --delay-check ($delay_check). This email, and the pointer to the paragraph buried in the docs, shared by Tom, are the only two ways one can understand what is causing this failure, and how to get around it. Naive question: What's stopping us from rewriting the code as follows. make(); make_install(); make_check(); ... other steps ... ... other steps-2... # or move make_check() call here With a quick google search I could not find why --delay-check is necessary on Apline linux, as well; can you please elaborate. Best regards, Gurjeet http://Gurje.et
