Re: Fix output of zero privileges in psql
On Monday, October 23, 2023, Laurenz Albe wrote:
> On Mon, 2023-10-23 at 11:37 -0700, David G. Johnston wrote:
> > > I didn't understand this completely. You want default privileges
> displayed as
> > > "(default)", but are you for or against "\pset null" to have its
> normal effect on
> > > the output of backslash commands in all other cases?
> >
> > I haven’t inspected other cases but to my knowledge we don’t typically
> represent
> > non-unknown things using NULL so I’m not expecting other places to have
> this
> > representation problem.
>
> The first example that comes to my mind is the "ICU Locale" and the "ICU
> Rules"
> in the output of \l. There are many others.
Both of those fall into “this null means there is no value for these
(because we aren’t using icu)”. I have no qualms with leaving true nulls
represented as themselves. Clean slate maybe I print “(not using icu)”
there instead of null but it isn’t worth the effort to change.
>
> > I won’t argue that exposing such NULLS is wrong, just it would
> preferable IME
> > to avoid doing so. NULL means unknown or not applicable and default
> privileges
> > are neither of those things. I get why our catalogs choose such an
> encoding and
> > agree with it, and users that find the need to consult the catalogs will
> need to
> > learn such details. But we should strive for them to be able to survive
> with
> > psql meta-commands.
>
> Sure, it would be best to hide this implementation detail from the user.
> The correct way to do that would be to fake an ACL entry like
> "laurenz=arwdDxt/laurenz"
> if there is a NULL in the catalog, but that would add a ton of special-case
> code to psql, which does not look appealing at all.
More generically it would be “[PUBLIC=]/???/postgres” and
{OWNER}=???/postgres
It would ideally be a function call for psql and a system info function
usable for anyone.
David J.
Re: Fix output of zero privileges in psql
On Monday, October 23, 2023, Laurenz Albe wrote: > On Mon, 2023-10-23 at 08:35 -0700, David G. Johnston wrote: > > > along with not translating (none) and (default) and thus making the data > contents > > of these views environment independent. But minimizing the variance of > these command's > > output across systems doesn't seem like a design goal that is likely to > gain consensus > > and is excessive when viewed within the framework of these being only > for human consumption. > > I didn't understand this completely. You want default privileges > displayed as > "(default)", but are you for or against "\pset null" to have its normal > effect on > the output of backslash commands in all other cases? I haven’t inspected other cases but to my knowledge we don’t typically represent non-unknown things using NULL so I’m not expecting other places to have this representation problem. I don’t think any of our meta-command outputs should modify pset null. Left join cases should be considered unknown, represented as NULL, and obey the user’s setting. I do believe that we should be against exposing, like in this case, any internal implementation detail that encodes something (e.g., default privileges) as NULL in the catalogs, to the user of the psql meta-commands. I won’t argue that exposing such NULLS is wrong, just it would preferable IME to avoid doing so. NULL means unknown or not applicable and default privileges are neither of those things. I get why our catalogs choose such an encoding and agree with it, and users that find the need to consult the catalogs will need to learn such details. But we should strive for them to be able to survive with psql meta-commands. David J.
Re: Fix output of zero privileges in psql
On Mon, Oct 23, 2023 at 7:57 AM Tom Lane wrote: > > IOW, the current definition is "NULL privileges print as an empty > string no matter what", and I don't think that serves to reduce > confusion about whether an ACL is NULL or not. We ought to be doing > what we can to make clear that such an ACL *is* NULL, because > otherwise people won't understand how it differs from an empty ACL. > > I tend to prefer the argument that these views are for human consumption and should be designed with that in mind. Allowing the user to decide whether they prefer NULL to print as the empty string or something else works within that framework. And the change of behavior for everyone with a non-default \pset gets accepted under that framework as well. I would suggest that we make the expected presence of NULL as an input explicit: case when spcacl is null then null when cardinality(spcacl) = 0 then '(none)' -- so as not to confuse it with null being printed also as an empty string else array_to_string(spcacl, E'\\n') end as "Access privileges" I would offer up: when spcacl is null then '(default)' along with not translating (none) and (default) and thus making the data contents of these views environment independent. But minimizing the variance of these command's output across systems doesn't seem like a design goal that is likely to gain consensus and is excessive when viewed within the framework of these being only for human consumption. David J.
Re: Fix output of zero privileges in psql
On Monday, October 23, 2023, Laurenz Albe wrote:
> On Mon, 2023-10-23 at 07:03 -0700, David G. Johnston wrote:
> > On Monday, October 23, 2023, Laurenz Albe
> wrote:
> > >
> > > --- a/src/bin/psql/describe.c
> > > +++ b/src/bin/psql/describe.c
> > > @@ -6718,7 +6680,13 @@ static void
> > >printACLColumn(PQExpBuffer buf, const char *colname)
> > >{
> > > appendPQExpBuffer(buf,
> > > - "pg_catalog.array_to_string(%s, E'\\n') AS
> \"%s\"",
> > > + "CASE\n"
> > > + " WHEN %s IS NULL THEN ''\n"
> > > + " WHEN pg_catalog.cardinality(%s) = 0 THEN
> '%s'\n"
> > > + " ELSE pg_catalog.array_to_string(%s,
> E'\\n')\n"
> > > + "END AS \"%s\"",
> > > + colname,
> > > + colname, gettext_noop("(none)"),
> > > colname, gettext_noop("Access privileges"));
> > >}
> > >
> > > This erroneously displays NULL as empty string and subverts my changes.
> > > I have removed the first branch of the CASE expression.
> >
> > There is no error here, the current consensus which this patch
> implements is to
> > not change the documented “default privileges display as blank”. We are
> solving
> > the empty privileges are not distinguishable complaint by printing
> (none) for them.
>
> Erik's latest patch included my changes to display NULL as NULL in psql,
> so that "\pset null" works as expected.
>
No, per the commit message, issuing an explicit \pset null is a kludge and
it gets rid of the hack in favor of making the query itself produce an
empty string. i.e., we choose a poor implementation to get the documented
behavior and we are cleaning that up as an aside to the main fix.
Changing the behavior so that default privileges print in correspondence to
the setting of \pset null is, IME, off the table for this patch. Its one
and only goal is to reliably distinguish empty and default privileges.
That is our extant bug.
We document default privileges print as an empty string - I do not think we
should change the definition to "default privileges print null which can be
controlled via \pset null", and regardless of preference doing so is not a
bug.
David J.
Re: Fix output of zero privileges in psql
On Monday, October 23, 2023, Laurenz Albe wrote:
>
> --- a/src/bin/psql/describe.c
> +++ b/src/bin/psql/describe.c
> @@ -6718,7 +6680,13 @@ static void
>printACLColumn(PQExpBuffer buf, const char *colname)
>{
> appendPQExpBuffer(buf,
> - "pg_catalog.array_to_string(%s, E'\\n') AS
> \"%s\"",
> + "CASE\n"
> + " WHEN %s IS NULL THEN ''\n"
> + " WHEN pg_catalog.cardinality(%s) = 0 THEN '%s'\n"
> + " ELSE pg_catalog.array_to_string(%s, E'\\n')\n"
> + "END AS \"%s\"",
> + colname,
> + colname, gettext_noop("(none)"),
> colname, gettext_noop("Access privileges"));
>}
>
> This erroneously displays NULL as empty string and subverts my changes.
> I have removed the first branch of the CASE expression.
>
There is no error here, the current consensus which this patch implements
is to not change the documented “default privileges display as blank”. We
are solving the empty privileges are not distinguishable complaint by
printing (none) for them.
David J.
Re: Fix output of zero privileges in psql
On Fri, Oct 20, 2023 at 7:29 PM Erik Wienhold wrote: > On 2023-10-20 22:35 +0200, David G. Johnston wrote: > > In short, I don't want default privileges to start to obey \pset null > when > > it never has before and is documented as displaying the empty string. I > do > > want the empty string produced by empty privileges to change to (none) so > > that it no longer is indistinguishable from our choice of presentation > for > > the default privilege case. > > I haven't thought off this yet. The attached v3 of my initial patch > does that. It also includes Laurenz' fix to no longer ignore \pset null > (minus the doc changes that suggest using \pset null to distinguish > between default and empty privileges because that's no longer needed). > > Thank you. It looks good to me as-is, with one possible nit. I wonder if it would be clearer to say: "If the Access privileges column is *blank* for a given object..." instead of "empty" to avoid having both "empty [string]" and "empty privileges" present in the same paragraph and the empty string not pertaining to the empty privileges. David J.
Re: Fix output of zero privileges in psql
On Fri, Oct 20, 2023 at 12:57 PM Tom Lane wrote:
> "David G. Johnston" writes:
> > On Fri, Oct 20, 2023 at 12:34 PM Tom Lane wrote:
> >> As near as I can tell, doing both things (the \pset null fix and
> >> substituting "(none)" for empty privileges) would be an acceptable
> >> answer to everyone who has commented. Let's proceed with both
> >> patches, or combine them into one if there are merge conflicts.
>
> > I'm under the impression that removing the null representation of empty
> > privileges by making them (none) removes all known \d commands that
> output
> > nulls and don't obey \pset null.
>
> How so? IIUC the proposal is to substitute "(none)" for empty-string
> ACLs, not null ACLs. The \pset change should be addressing an
> independent case.
>
Ok, I found my mis-understanding and better understand where you are all
coming from now; I apparently had the usage of NULL flip-flopped.
Taking pg_tablespace as an example. Its "spcacl" column produces NULL for
default privileges and '{}'::text[] for empty privileges.
Thus, today:
empty: array_to_string('{}'::text[], '\n') produces an empty string
default: array_to_string(null, '\n') produces null which then was printed
as a hard-coded empty string via forcibly changing \pset null
I was thinking the two cases were reversed.
My proposal for changing printACLColumn is thus:
case when spcacl is null then ''
when cardinality(spcacl) = 0 then '(none)'
else array_to_string(spcacl, E'\\n')
end as "Access privileges"
In short, I don't want default privileges to start to obey \pset null when
it never has before and is documented as displaying the empty string. I do
want the empty string produced by empty privileges to change to (none) so
that it no longer is indistinguishable from our choice of presentation for
the default privilege case.
Mechanically, we remove the existing \pset null for these metacommands and
move it into the query via the added CASE expression in the printACLColumn
function.
This gets rid of NULL as an output value for this column and makes the
patch regarding \pset null discussion unnecessary. And it leaves the
existing well-established empty string for default privileges alone (and
changing this is what I believe Tom is against and I agree on that point).
David J.
Re: Fix output of zero privileges in psql
On Fri, Oct 20, 2023 at 12:34 PM Tom Lane wrote: > Laurenz Albe writes: > > I am not sure how to proceed. Perhaps it would indeed be better to have > > two competing commitfest entries. Both could be "ready for committer", > > and the committers can decide what they prefer. > > As near as I can tell, doing both things (the \pset null fix and > substituting "(none)" for empty privileges) would be an acceptable > answer to everyone who has commented. Let's proceed with both > patches, or combine them into one if there are merge conflicts. > > I'm under the impression that removing the null representation of empty privileges by making them (none) removes all known \d commands that output nulls and don't obey \pset null. At least, the existing \pset null patch doesn't purport to fix anything that would become not applicable if the (none) patch goes in. I.e., at present they are mutually exclusive. David J.
Re: The danger of deleting backup_label
On Thursday, October 19, 2023, David Steele wrote: > On 10/19/23 10:24, Robert Haas wrote: > >> On Wed, Oct 18, 2023 at 7:15 PM David Steele wrote: >> >>> pg_llbackup -d $CONNTR --backup-label=PATH --tablespace-map=PATH --copy-data-directory=SHELLCOMMAND I think in most cases where this would be useful the user should just be >>> using pg_basebackup. If the backup is trying to use snapshots, then >>> backup_label needs to be stored outside the snapshot and we won't be >>> able to easily help. >>> >> >> Right, the idea of the above was that you would specify paths for the >> backup label and the tablespace map that were outside of the snapshot >> directory in that kind of case. But you couldn't screw up the line >> endings or whatever because pg_llbackup would take care of that aspect >> of it for you. >> > > What I meant here (but said badly) is that in the case of snapshot > backups, the backup_label and tablespace_map will likely need to be stored > somewhere off the server since they can't be part of the snapshot, perhaps > in a key store. In that case the backup software would still need to read > the files from wherever we stored then and correctly handle them when > storing elsewhere. If you were moving the files to say, S3, a similar thing > needs to happen. In general, I think a locally mounted filesystem is very > unlikely to be the final destination for these files, and if it is then > probably pg_basebackup is your friend. > We are choosing to not take responsibility if the procedure used by the dba is one that takes a full live backup of the data directory as-is and then brings that backup back into production without making any changes to it. That restored copy will be bootable but quite probably corrupted. That is on them as we have no way to make it non-bootable without risking source database being unable to be restarted automatically upon a crash. In short, a snapshot methodology is beyond what we are willing to provide protections for. What I do kinda gather from the above is we should be providing a pg_baserestore application if we want to give our users fewer reasons to write their own tooling against the API and/or want to add more complexity to pg_basebackup to handle various needs that need corresponding recovery needs that we should implement as code instead of documentation. David J.
Re: The danger of deleting backup_label
On Wednesday, October 18, 2023, David Steele wrote: > On 10/18/23 08:39, Robert Haas wrote: > >> On Tue, Oct 17, 2023 at 4:17 PM David Steele wrote: >> >>> Given that the above can't be back patched, I'm thinking we don't need >>> backup_label at all going forward. We just write the values we need for >>> recovery into pg_control and return *that* from pg_backup_stop() and >>> tell the user to store it with their backup. We already have "These >>> files are vital to the backup working and must be written byte for byte >>> without modification, which may require opening the file in binary >>> mode." in the documentation so dealing with pg_control should not be a >>> problem. pg_control also has a CRC so we will know if it gets munged. >>> >> >> Yeah, I was thinking about this kind of idea, too. I think it might be >> a good idea, although I'm not completely certain about that, either. >> > > > > First, anything that is stored in the backup_label but not the control >> file has to (a) move into the control file, >> > > I'd rather avoid this. > > If we are OK outputting custom pg_control file content from pg_backup_end to govern this then I’d probably just include “tablespace_map_required=true|false” and “backup_label_required=true” lines in it and leave everything else mostly the same, including the name. In order for a server with those lines in its pg_control to boot it requires that a signal file be present along with any of the named files where *_required is true. Upon successful completion those lines are removed from pg_control. It seem unnecessary to move any and all relevant content into pg_control; just a flag to ensure that those files that are needed a present in the backup directory and whatever validation of those files is needed to ensure they provide sufficient data. If the user removes those files without a backup the server is not going to start unless they make further obvious attempts to circumvent the design. Manually editing pg_comtrol being obvious circumventing. This would seem to be a compatible change. If you fail to use the pg_control from pg_backup_stop you don’t get the safeguard but everything still works. Do we really believe we need to break/force-upgrade tooling to use this new procedure? Depending on the answer to the torn pg_comtrol file problem which may indeed warrant such breakage. David J.
Re: Improving Physical Backup/Restore within the Low Level API
On Tue, Oct 17, 2023 at 12:30 PM David Steele wrote: > On 10/17/23 14:28, Robert Haas wrote: > > On Mon, Oct 16, 2023 at 5:21 PM David G. Johnston > > wrote: > >> But no, by default, and probably so far as pg_basebackup is concerned, > a server crash during backup results in requiring outside intervention in > order to get the server to restart. > > > > Others may differ, but I think such a proposal is dead on arrival. As > > Laurenz says, that's just reinventing one of the main problems with > > exclusive backup mode. > > I concur -- this proposal resurrects the issues we had with exclusive > backups without solving the issues being debated elsewhere, e.g. torn > reads of pg_control or users removing backup_label when they should not. > > Thank you all for the feedback. Admittedly I don't understand the problem of torn reads well enough to solve it here but I figured by moving the "must not remove" stuff out of backup_label and into pg_control the odds of it being removed from the backup and the backup still booting basically go to zero. I do agree that renaming backup_label to something like "recovery_stuff_do_not_delete.conf" probably does that just as well without the downside. Placing a copy of all relevant files into pg_backup_metadata seems like a decent shield against accidents and a way to reliably self-document the backup even if the behavioral changes are not desired. Though doing that and handling multiple concurrent backups probably makes the cost too high to move away from relying just on documentation. I suppose I'd consider having to add one file to the data directory to be an improvement over having to remove two of them - in terms of what it takes to recover from system failure during a backup. David J
Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"
On Tue, Oct 17, 2023 at 10:50 AM Robert Haas wrote: > Life would be a lot easier here if we could get rid of the low-level > backup API and just have pg_basebackup DTWT, but that seems like a > completely non-viable proposal. > Yeah, my contribution to this area [1] is focusing on the API because I figured we've provided it and should do our best to have it do as much as possible for the dba or third-parties that build tooling on top of it. I kinda think that adding a pg_backup_metadata directory that pg_backup_start|stop can use may help here. I'm wondering whether those functions provide enough control guarantees that pg_control's "in_backup=true|false" flag proposed in that thread is reliable enough when copied to the root directory in the backup. I kinda feel that so long as the flag is reliable it should be possible for the signal file processing code to implement whatever protocol we need. David J. [1] https://www.postgresql.org/message-id/CAKFQuwbpz4s8XP_%2BKhsif2eFaC78wpTbNbevUYBmjq-UCeNL7Q%40mail.gmail.com
Re: Restoring default privileges on objects
On Fri, Oct 6, 2023 at 1:29 PM Laurenz Albe wrote: > On Fri, 2023-10-06 at 22:18 +0200, Laurenz Albe wrote: > > On Fri, 2023-10-06 at 22:16 +0200, Laurenz Albe wrote: > > > Here is a patch that does away with the special handling of NULL values > > > in psql backslash commands. > > > > Erm, I forgot to attach the patch. > > I just realize that there is a conflicting proposal. I'll reply to that > thread. > Sorry for the noise. > > This thread seems officially closed and the discussion moved to [1]. Over there, after reading both threads, I am seeing enough agreement that changing these queries to always print "(none)" (translating the word none) where today they output null, and thus plan to move forward with the v1 patch on that thread proposing to do just that. Please chime in over there on this specific option - whether you wish to support or reject it. Should it be rejected the plan is to have these queries respect the user's preference in \pset null. Please make it clear if you would rather maintain the status quo against either of those two options. Thanks! David J. [1] https://www.postgresql.org/message-id/ab67c99bfb5dea7bae18c77f96442820d19b5448.camel%40cybertec.at
Re: Fix output of zero privileges in psql
On Mon, Oct 16, 2023 at 6:19 PM Laurenz Albe wrote: > On Mon, 2023-10-16 at 23:51 +0200, Erik Wienhold wrote: > > What's the process for the CommitFest now since we settled on your > > patch? This is my first time being involved in this, so still learning. > > I'see that you've withdrawn your initial patch [1] but this thread is > > still attached to my patch [2]. Should I edit my CF entry (or withdraw > > it) and you reactivate yours? > > I don't think it makes sense to have two competing commitfest entries, > and we should leave it on this thread. If you are concerned about > authorship, we could both be mentioned as co-authors, if the patch ever > gets committed. > > I'd still like to wait for feedback from David before I change anything. > > Reading both threads I'm not seeing any specific rejection of the solution that we simply represent empty privileges as "(none)". I see an apparent consensus that if we do continue to represent it as NULL that the printout should respect \pset null. Tom commented in favor of (none) on the other thread with some commentary regarding how extremely rare it is; then turns around and is "fairly concerned" about changing the current blank presentation of its value which is going to happen for some people regardless of which approach is chosen. (idk...maybe in favor of (none)) Peter's comment doesn't strike me as recognizing that (none) is even an option on the table...(n/a) Stuart, the original user complainant, seems to favor (none) Erik seems to favors (none) I favor (none) It's unclear to me whether you Laurenz are against (none) or just trying to go with the group consensus that doesn't actually seem to be against (none). I'm in favor of iterating on v1 on this thread (haven't read it yet) and presenting it as the proposed solution. If that ends up getting shot down we can revive v5 (still need to review as well). We should probably post on that thread that this one exists and post a link to it. David J.
Re: Improving Physical Backup/Restore within the Low Level API
On Mon, Oct 16, 2023 at 12:36 PM David G. Johnston < david.g.johns...@gmail.com> wrote: > On Mon, Oct 16, 2023 at 12:09 PM Laurenz Albe > wrote: > >> I think it won't meet with favor if there are cases that require manual >> intervention >> for starting the server. That was the main argument for getting rid of >> the exclusive >> backup API, which had a similar problem. >> > > In the rare case of a crash of the source database while one or more > databases are in progress. > Or even more simply, just document that should the process executing pg_backup_start, and eventually pg_backup_end, that noticed its session die out from under it, should just add crash.signal to the data directory (there probably can be a bit more intelligence involved in case the session crash was isolated). A normal server shutdown should remove any crash.signal files it sees (and ensure in_backup="false"...). A non-normal shutdown is going to end up in crash recovery anyway so having the signal file there won't harm anything even if pg_control is showing "in_backup=false". In short, I probably don't know the details well enough to code the solution but this seems solvable for those users that need automatic reboot and crash recovery during an incomplete backup. But no, by default, and probably so far as pg_basebackup is concerned, a server crash during backup results in requiring outside intervention in order to get the server to restart. It specifically requires creation of crash.signal, the specific method being unimportant and its contents being fixed - whether empty or otherwise. David J.
Re: Improving Physical Backup/Restore within the Low Level API
On Mon, Oct 16, 2023 at 12:09 PM Laurenz Albe wrote: > I think it won't meet with favor if there are cases that require manual > intervention > for starting the server. That was the main argument for getting rid of > the exclusive > backup API, which had a similar problem. > In the rare case of a crash of the source database while one or more databases are in progress. Restoring the backup requires manual intervention with signal files today. I get a desire for the live production server to not need intervention to recover from a crash but I can't help but feel that this requirement plus the goal of making this a non-interventionist as possible during recovery are incompatible. But I haven't given it a great amount of thought as I felt the limited scope and situation were an acceptable cost for keeping the process straight-forward (i.e., starting up a backup mode instance requires a signal file that dictates the kind of recovery to perform). We can either make the live backup contents invalid until something happens after pg_backup_stop ends that makes it valid or we have to make the current system being backed up invalid so long as it's in backup mode. The later seemed easier and doesn't require actions outside of our control. > Also, how do you envision two concurrent backups with your setup? > I don't know if I understand the question - if ensuring that "in backup" is turned on when the first backup starts and is turned off when the last backup ends isn't sufficient for concurrent usage I don't know what else I need to deal with. Apparently concurrent backups already work today and I'm not seeing how, aside from the process ids for the metadata directories (i.e., the user needs to remove all but their own process subdirectory from pg_backup_metadata) and state flag they wouldn't continue to work as-is. David J.
Re: Improving Physical Backup/Restore within the Low Level API
On Mon, Oct 16, 2023 at 10:26 AM Laurenz Albe wrote: > On Mon, 2023-10-16 at 09:26 -0700, David G. Johnston wrote: > > This email is a first pass at a user-visible design for how our backup > and restore > > process, as enabled by the Low Level API, can be modified to make it > more mistake-proof. > > In short, it requires pg_start_backup to further expand upon what it > means for the > > system to be in the midst of a backup, pg_stop_backup to reverse those > things, > > and modifying the startup process to deal with the server having crashed > while the > > system is in that backup state. Notes at the end extend the design to > handle concurrent backups. > > > > The core functional changes are: > > 1) pg_backup_start modifies a newly added "in backup" state flag in > pg_control to on. > > 2) pg_backup_stop modifies that flag back to off. > > 3) postmaster will refuse to start if that flag is on, unless one of: > > a) crash.signal exists in the data directory > > b) recovery.signal exists in the data directory > > c) standby.signal exists in the data directory > > 4) Signal file processing causes the in-backup flag in pg_control to be > set to off > > > > The newly added crash.signal file is required to handle the case where > the server > > crashes after pg_backup_start and before pg_backup_stop. It initiates a > crash recovery > > of the instance just as is done today but with the added change of > flipping the flag > > to off when recovery is complete just before going live. > > I see a couple of problems and/or things that need clarification with that > idea: > > - Two backups can run concurrently. How do you reconcile that with the > "in backup" > flag and crash.signal? > - I guess crash.signal is created during pg_start_backup(). So that file > will be > included in the backup. How do you handle that during recovery? Ignore > it if > another signal file is present? And if the user forgets to create a > signal file > for recovery, how do you prevent PostgreSQL from performing crash > recovery? > > crash.signal is created in the pg_backup_metadata directory, not the root directory. Should the server crash while any backup is in progress pg_control would be aware of that fact (in_backup=true would still be there, instead of in_backup=false which only comes back after all backups have completed) and the server will not restart without user intervention - specifically, moving the crash.signal file from (one of) the pg_backup_metadata subdirectories to the root directory. As there is nothing special about the crash.signal files in the pg_backup_metadata subdirectories "touch crash.signal" could be used. The backed up pg_control file will have in_backup=true (I haven't pondered the torn reads dynamic of this - I'm supposing that placing a copy of pg_control into the pg_backup_metadata directory might be part of solving that problem). David J.
Improving Physical Backup/Restore within the Low Level API
Hi! This email is a first pass at a user-visible design for how our backup and restore process, as enabled by the Low Level API, can be modified to make it more mistake-proof. In short, it requires pg_start_backup to further expand upon what it means for the system to be in the midst of a backup, pg_stop_backup to reverse those things, and modifying the startup process to deal with the server having crashed while the system is in that backup state. Notes at the end extend the design to handle concurrent backups. The core functional changes are: 1) pg_backup_start modifies a newly added "in backup" state flag in pg_control to on. 2) pg_backup_stop modifies that flag back to off. 3) postmaster will refuse to start if that flag is on, unless one of: a) crash.signal exists in the data directory b) recovery.signal exists in the data directory c) standby.signal exists in the data directory 4) Signal file processing causes the in-backup flag in pg_control to be set to off The newly added crash.signal file is required to handle the case where the server crashes after pg_backup_start and before pg_backup_stop. It initiates a crash recovery of the instance just as is done today but with the added change of flipping the flag to off when recovery is complete just before going live. The error message for the failed startup while in backup will tell the dba that one of the three signal files must exist. When processing recovery.signal or standby.signal the presence of the backup_label and tablespace_map files are mandatory and the system will also fail to start should they be missing. For non-functional changes I would also suggest doing the following: pg_backup_start will create a "pg_backup_metadata" directory if there is not already one, or will empty it if there is. pg_backup_start will create a crash.signal file in that directory pg_backup_stop will create files within pg_backup_metadata upon its completion: backup_label tablespace_map recovery.signal standby.signal All of the instructions regarding what to place in those files should be removed and instead the system should write them - no copy-paste. The instructions modified to say "copy the backup_label and tablespace_map files to the root of the backup directory and the recovery and standby signal files to the pg_backup_metadata directory in the backup. Additionally, we document crash recovery by saying "move crash.signal from pg_backup_metadata to the root of the data directory". We should explicitly advise excluding or removing pg_backup_metadata/crash.signal from the backup as well. Extending the above to handle concurrent backup, for pg_control we'd sill use the on/off flag but we have to have a shared in-memory session lock on something so that only the last surviving process actually changes it to off while also dealing with sessions that terminate without issuing pg_backup_stop and without the server itself crashing. (I'm unfamiliar with how this is handled today but I presume a mechanism exists already that just needs to be extended). For the non-functional stuff, pg_backup_start returns a process id, and subdirectories under pg_backup_metadata are created named with such. Add a pg_backup_cleanup() function that executes while not in backup mode to clean up those subdirectories. Any subdirectory in the backup that isn't the specified process id from pg_start_backup should be excluded/removed. David J.
Re: Pro et contra of preserving pg_proc oids during pg_upgrade
On Thu, Oct 12, 2023 at 2:58 PM Nikita Malakhov wrote: > Why pg_upgrade cannot be used? > We document both a pg_dump/pg_restore migration and a pg_upgrade one (not to mention that logical backup and restore would cause the oids to change). It seems odd to have a feature that requires pg_upgrade to be the chosen one. pg_upgrade is an option, not a requirement. Same goes for pg_basebackup. pg_upgrade itself warns that should the on-disk file format change then it would be unusable - though I suspect that we'd end up with some kind of hybrid approach in that case. > OID preservation logic is already implemented > for several OIDs in catalog tables, like pg_class, type, relfilenode, > enum... > > We are allowed to preserve oids if we wish but that doesn't mean we must, nor does doing so constitute a declaration that such oids are part of the public API. And I don't see us making OIDs part of the public API unless we modify pg_dump to include them in its output. > Actually, I've asked here because there are several references to PG_PROC > oids > from other tables in the system catalog > Of course there are, e.g., views depending on functions would result is those. But pg_upgrade et al. recomputes the views so the changing of oids isn't a problem. Long text fields are common in databases; and if there are concerns with parsing/interpretation we can add functions to make doing that simpler. David J.
Re: Pro et contra of preserving pg_proc oids during pg_upgrade
On Thu, Oct 12, 2023 at 1:31 PM Nikita Malakhov wrote: > About using surrogate key - this feature is more for data generated by > the DBMS itself, i.e. data processed by some extension and saved > and re-processed automatically or by user's request, but without bothering > user with these internal keys. > Then what does it matter whether you spell it: 12345 or my_ext.do_something(int) ? Why do you require us to redefine the scope for which pg_proc.oid is useful in order to implement this behavior? Your extension breaks if your user uses logical backups or we otherwise get into a position where pg_upgrade cannot be used to migrate in the future. Is avoiding the textual representation so necessary that you need to add another dependency to the system? That just seems unwise regardless of how easy it may be to accomplish. David J.
Re: Pro et contra of preserving pg_proc oids during pg_upgrade
On Thu, Oct 12, 2023 at 11:43 AM Robert Haas wrote: > On Thu, Oct 12, 2023 at 2:38 PM David G. Johnston > wrote: > > It's more like a lot number or surveying tract than an postal address. > Useful for a single party, the builder or the government, but not something > you give out to other people so they can find you. > > > > Whether or not we copy over oids should be done based upon our internal > needs, not end users. Which is why the fee that do get copied exists, > because we store them in internal files that we want to copy as part of the > upgrade. It also isn't like pg_dump/restore is going to retain them and > the less divergence between that and pg_upgrade arguably the better. > > We build the product for the end users. Their desires and needs are > relevant. And if they're telling us we did it wrong, we need to listen > to that. We don't have to do everything that everybody wants, but > treating developer needs as strictly more important than end-user > needs is self-defeating. > Every catalog has both a natural and a surrogate key. Developers get to use the surrogate key while end-users get to use the natural one (i.e., the one they provided). I see no reason to change that specification. And I do believe there are no compelling reasons for an end-user to need to use the surrogate key instead of the natural one. The example provided by the OP isn't one, IMO, the overall goal can be accomplished via the natural key (if it cannot, maybe we need to make retrieving the natural key for a pg_proc record given an OID easier). The fact that OIDs are not even accessible via SQL further reinforces this belief. The only reason to need OIDs as a DBA is to perform joins among the catalogs and all such joins are local to the database and even session executing them - the specific values are immaterial. The behavior of pg_upgrade only preserving OIDs that are necessary due to the physical copying of data files from the old server to the new one seems sufficient both in terms of effort and the principle of doing the minimum amount to solve the problem at hand. David J.
Re: Pro et contra of preserving pg_proc oids during pg_upgrade
On Thu, Oct 12, 2023, 11:21 Robert Haas wrote: > > The pg_upgrade experience right now is a bit as if you woke up in the > morning and found that city officials came by during the night and > renumbered your house, thus changing your address. Then, they sent > change of address forms to everyone who ever mails you anything, plus > updated your address with your doctor's office and your children's > school. In a way, there's no problem: nothing has really changed for > you in any way that matters. Yet, I think that would feel pretty > uncomfortable if it actually happened to you, and I think the > pg_upgrade experience is uncomfortable in the same way. > It's more like a lot number or surveying tract than an postal address. Useful for a single party, the builder or the government, but not something you give out to other people so they can find you. Whether or not we copy over oids should be done based upon our internal needs, not end users. Which is why the fee that do get copied exists, because we store them in internal files that we want to copy as part of the upgrade. It also isn't like pg_dump/restore is going to retain them and the less divergence between that and pg_upgrade arguably the better. David J. >
Re: Pro et contra of preserving pg_proc oids during pg_upgrade
On Thu, Oct 12, 2023 at 7:36 AM Tom Lane wrote: > Nikita Malakhov writes: > > Please advise on the idea of preserving pg_proc oids during pg_upgrade, > in > > a way like relfilenodes, type id and so on. What are possible downsides > of > > such a solution? > > You have the burden of proof backwards. That would add a great deal > of new mechanism, and you haven't provided even one reason why it'd > be worth doing. > > I was curious about the comment regarding type oids being copied over and I found the commentary in pg_upgrade.c that describes which oids are copied over and why, but the IMPLEMENTATION seems to be out-of-sync with the actual implementation. """ It preserves the relfilenode numbers so TOAST and other references to relfilenodes in user data is preserved. (See binary-upgrade usage in pg_dump). We choose to preserve tablespace and database OIDs as well. """ David J.
Re: Pro et contra of preserving pg_proc oids during pg_upgrade
On Thu, Oct 12, 2023 at 9:57 AM Nikita Malakhov wrote: > Say, we have data processed by some user function and we want to keep > reference to this function > in our data. > Then you need to keep the user-visible identifier of said function (schema+name+input argument types - you'd probably want to incorporate version into the name) in your user-space code. Exposing runtime generated oids to user-space is not something I can imagine the system supporting. It goes against the very definition of "implementation detail" that user-space code is not supposed to depend upon. David J.
Re: CHECK Constraint Deferrable
On Mon, Oct 9, 2023 at 1:27 PM Robert Haas wrote: > On Tue, Oct 3, 2023 at 10:05 AM David G. Johnston > wrote: > >> The real-world use case, at least for me, is when using an ORM. For > large object-graphs ORMs have a tendency to INSERT first with NULLs then > UPDATE the “NOT NULLs” later. > >> > >> “Rewrite the ORM” is not an option for most of us… > > > > Between this and Vik comment it sounds like we should probably require a > patch in this area to solve both the not null and check constraint deferral > omissions then, not just one of them (alternatively, let’s solve the not > null one first). > > I have a couple of problems with this comment: > > 1. I don't know which of Vik's comments you're talking about here, but > it seems to me that Vik is generally in favor of this feature, so I'm > a bit surprised to hear that one of his comments led you to think that > it should be burdened with additional requirements. > Specifically, Vik commented that the standard requires implementing NOT NULL as a check constraint and thus needs to allow for deferrable check constraints in order for not null checks to be deferrable. I agree fully that deferring a not null check makes for an excellent use case. But we've also decided to make NOT NULL its own thing, contrary to the standard. Thus my understanding for why this behavior is standard mandated is that it is to allow for deferrable not null constraints and thus our goal should be to make our not null constraints deferrable. The only other example case of wanting a deferrable check constraint involved the usage of a function that we expressly prohibit as a check constraint. The argument, which I weakly support, is that if our adding deferrable check constraints increases the frequency of such functions being created and used by our users, then we should continue to prohibit such deferrability and require those users to properly implement triggers which can then be deferred. With a deferrable not null constraint any other reasonable check constraints can simply evaluate to null during the period where they should be deferred - because their column inputs are deferred nulls - and then we be fully evaluated when the inputs ultimately end up non-null. 2. I don't think it's a good idea for the same patch to try to solve > two problems unless they are so closely related that solving one > without solving the other is not sensible. A NOT NULL constraint apparently is just a special case of a check constraint which seems closely related enough to match your definition. But I guess you are right, I was trying to say no to this patch, and yes to the not null deferral idea, without being so explicit and final about it. While the coders are welcome to work on whatever they wish, the effort spent on this just doesn't seem that valuable compared to what is already in the queue being worked on needing reviews and commits. I can live with a gap in our standards conformance here since I haven't observed any uses cases that are impossible to accomplish except by adding this specific feature which would only cover NOT NULL constraints if the syntactical form for creating them were not used (which I suppose if we fail to provide NOT NULL DEFERRABLE that would argue for at least giving this work-around...) David J.
Re: Fix output of zero privileges in psql
On Mon, Oct 9, 2023 at 12:13 PM Tom Lane wrote: > Laurenz Albe writes: > > On Mon, 2023-10-09 at 09:30 -0700, David G. Johnston wrote: > >> My point with the second paragraph is that we could, instead of > documenting the > >> caveat about null printing as empty strings is to instead issue an > implicit > >> "\pset null ''" as part of these commands, and a "\pset null" > afterward, > >> conditioned upon it not already being set to a non-empty value. IOW, > the > >> special-casing we do today but actually do it in a way that > distinguishes the > >> two cases instead of forcing them to be indistinguishable. > > > -1 > > > The whole point of this patch is to make psql behave consistently with > respect to > > NULLs in meta-commands. Your suggestion would subvert that idea. > > Yeah. There is a lot of attraction in having \pset null affect these > displays just like all other ones. The fact that they act differently > now is a wart, not something we should replace with a different special > case behavior. > > Also, I'm fairly concerned about not changing the default behavior here. > The fact that this behavior has stood for a couple dozen years without > many complaints indicates that there's not all that big a problem to be > solved. I doubt that a new default behavior will be well received, > even if it's arguably better. > My position is that the default behavior should be changed such that the distinction these reports need to make between empty privileges and default privileges is impossible for the user to remove. I suppose the best direct solution given that goal is for the query to simply do away with any reliance on NULL being an indicator. Output an i18n'd "no permissions present" line in the rare empty permissions situation and leave the empty string for built-in default. If the consensus is to not actually fix these views to make them environment invariant in their accuracy then so be it. Having them obey \pset null seems like a half-measure but it at least is an improvement over the status quo such that users are capable of altering their system to make the reports distinguish the two cases if they realize the need. I do agree that my suggestion regarding the implicit \pset null, while plausible, leaves the wart that NULL is being used to mean something specific. Better is to just use a label for that specific thing. David J.
Re: Fix output of zero privileges in psql
On Mon, Oct 9, 2023 at 1:29 AM Laurenz Albe wrote: > On Sun, 2023-10-08 at 19:58 -0700, David G. Johnston wrote: > > > The built-in default privileges are only in effect if the object has not > been > > the target of a GRANT or REVOKE and also has not had its default > privileges > > modified using ALTER DEFAULT PRIVILEGES. (???: if it is possible to > revert > > back to the state of built-in privileges that would need to be described > here.) > > I don't think that we need to mention ALTER DEFAULT PRIVILEGES there. If > the default privileges have been altered, the ACL will not be stored as > NULL in the catalogs. > It's already mentioned there, I just felt bringing the mention of ADP and grant/revoke both invalidating the built-in default privileges closer together would be an improvement. > > > The above removes the parenthetical regarding null in the catalogs, this > is > > intentional as it seems that the goal here is to use psql instead of the > > catalogs and adding its use of null being printed as the empty string > just > > seems likely to add confusion. > > To me, mentioning the default privileges are stored as NULLs in the > catalogs > is not an invitation to view the privileges with catalog queries, but > information about implementation details that explains why default > privileges > are displayed the way they are. > Calling it an implementation detail leads me to conclude the point does not belong in the user-facing administration documentation. > > > > > Perhaps it would also be good to mention this in the psql > documentation. > > > > We've chosen a poor default and I'd rather inform the user of specific > meta-commands > > to be wary of this poor default and change it at the point they will be > learning > > about the meta-commands adversely affected. > > > > That said, I'd be willing to document that these commands, because they > are affected > > by empty string versus null, require a non-empty-string value for \pset > null and will > > choose the string '' for the duration of the meta-command's > execution if the > > user's setting is incompatible. > > I am not certain I understood you correctly. > Are you advocating for adding a mention of "\pset null" to every backslash > command > that displays privileges? That is excessive, in my opinion. > Yes, I am. I suppose the argument that any user of those commands is expected to have read the ddl/privileges chapter would suffice for me though. My point with the second paragraph is that we could, instead of documenting the caveat about null printing as empty strings is to instead issue an implicit "\pset null ''" as part of these commands, and a "\pset null" afterward, conditioned upon it not already being set to a non-empty value. IOW, the special-casing we do today but actually do it in a way that distinguishes the two cases instead of forcing them to be indistinguishable. David J.
Re: REL_15_STABLE: pgbench tests randomly failing on CI, Windows only
On Sun, Oct 8, 2023 at 9:10 PM Noah Misch wrote: > > I didn't think of any phrasing that clearly explained things without the > reader consulting the code. I considered these: > > "socket file descriptor out of range: %d" [what range?] > > Quick drive-by...but it seems that < 0 is a distinctly different problem than exceeding FD_SETSIZE. I'm unsure what would cause the former but the error for the later seems like: error: "Requested socket file descriptor %d exceeds connection limit of %d", fd, FD_SETSIZE-1 hint: Reduce the requested number of concurrent connections In short, the concept of range doesn't seem applicable here. There is an error state at the max, and some invalid system state condition where the position within a set is somehow negative. These should be separated - with the < 0 check happening first. And apparently this condition isn't applicable when dealing with jobs in connect_slot? Also, I see that for connections we immediately issue FD_SET so this check on the boundary of the file descriptor makes sense. But the remaining code in connect_slot doesn't involve FD_SET so the test for the file descriptor falling within its maximum seems to be coming out of nowhere. Likely this is all good, and the lack of symmetry is just due to the natural progressive development of the code, but it stands out to the uninitiated looking at this patch. David J.
Re: Fix output of zero privileges in psql
On Sun, Oct 8, 2023 at 6:55 PM Erik Wienhold wrote:
> On 2023-10-08 06:14 +0200, Laurenz Albe write:
> > On Sat, 2023-10-07 at 20:41 +0200, Erik Wienhold wrote:
> > > > If you are happy enough with my patch, shall we mark it as ready for
> > > > committer?
> > >
> > > I amended your patch to also document the effect of \pset null in this
> > > case. See the attached v2.
> >
> > +1
> >
> > If you mention in ddl.sgml that you can use "\pset null" to distinguish
> > default from no privileges, you should mention that this only works with
> > psql. Many people out there don't use psql.
>
> I don't think this is necessary because that section in ddl.sgml is
> already about psql and \dp.
>
I agree that we are simply detailing how psql makes this information
available to the reader and leave users of other clients on their own to
figure out their own methods - which many clients probably have handled for
them anyway.
For us, I would suggest the following wording:
In addition to the situation of printing all acl items, the Access and
Column privileges columns report two other situations specially. In the
rare case where all privileges for an object have been explicitly removed,
including from the owner and PUBLIC, (i.e., has empty privileges) these
columns will display NULL. The other case is where the built-in default
privileges are in effect, in which case these columns will display the
empty string. (Note that by default psql will print NULL as an empty
string, so in order to visually distinguish these two cases you will need
to issue the \pset null meta-command and choose some other string to print
for NULLs). Built-in default privileges include all privileges for the
owner, as well as those granted to PUBLIC per for relevant object types as
described above. The built-in default privileges are only in effect if the
object has not been the target of a GRANT or REVOKE and also has not had
its default privileges modified using ALTER DEFAULT PRIVILEGES. (???: if it
is possible to revert back to the state of built-in privileges that would
need to be described here.)
The above removes the parenthetical regarding null in the catalogs, this is
intentional as it seems that the goal here is to use psql instead of the
catalogs and adding its use of null being printed as the empty string just
seems likely to add confusion.
> > Also, I'm not sure if "zero privileges" will be readily understood by
> > everybody. Perhaps "no privileges at all, even for the object owner"
> > would be a better wording.
>
> Changed in v3 to "empty privileges" with an explanation that this means
> "no privileges at all, even for the object owner".
>
+1
We probably should add the two terms to the glossary:
empty privileges: all privileges explicitly revoked from the owner and
PUBLIC (where applicable), and none otherwise granted.
built-in default privileges: owner having all privileges and no privileges
granted or removed via ALTER DEFAULT PRIVILEGES
> > Perhaps it would also be good to mention this in the psql documentation.
>
> Just once under \pset null with a reference to section 5.7? Something
> like "This is also useful for distinguishing default privileges from
> empty privileges as explained in Section 5.7."
>
> Or instead under every command affected by this change? \dp and \ddp
> already have such a reference ("The meaning of the privilege display is
> explained in Section 5.7.")
>
> I prefer the first one because it's less effort ;) Also done in v3.
>
We've chosen a poor default and I'd rather inform the user of specific
meta-commands to be wary of this poor default and change it at the point
they will be learning about the meta-commands adversely affected.
That said, I'd be willing to document that these commands, because they are
affected by empty string versus null, require a non-empty-string value for
\pset null and will choose the string '' for the duration of the
meta-command's execution if the user's setting is incompatible.
David J.
Re: Good News Everyone! + feature proposal
On Wednesday, October 4, 2023, Jon Erdman wrote: > > 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? > Run a cron job checking for them. Allow for overrides by adding a comment to any unclogged tables you’ve identified as being acceptable. David J.
Re: Various small doc improvements; plpgsql, schemas, permissions, oidvector
Extending my prior email which is now redundant. On Tue, Oct 3, 2023 at 7:00 PM David G. Johnston wrote: > On Tue, Oct 3, 2023 at 4:15 PM Karl O. Pinc wrote: > >> On Tue, 3 Oct 2023 14:51:31 -0700 >> "David G. Johnston" wrote: >> >> Isn't the entire section about "deviating from the normal flow of the >> code"? That's what makes me want "Exception" in the section title. >> > > It is about how error handling in a procedure diverts the flow from the > normal code path to some other code path - what that path is labelled is > less important than the thing that causes the diversion - an error. > > >> ? I remain (overly?) focused on the word "exception", since that's >> whats in the brain of the user that's writing RAISE EXCEPTION. >> It matters if exceptions and errors are different. If they're >> not, then it also matters since it's exceptions that the user's >> code raises. >> >> > It's unfortunate the keyword to raise the message level "ERROR" is > "EXCEPTION" in that command but I'd rather simply handle that one anomaly > that make the rest of the system use the word exception, especially seem to > be fairly consistent in our usage of ERROR already. I'm sympathetic that > other systems out there also encourage the usage of exception in this > context instead of error - but not to the point of opening up this > long-standing decision for rework. > > >> Have you any thoughts on the "permissions", "privleges" and >> "attributes" vocabulary/concepts used in this area? >> > > I think we benefit from being able to equate permissions and privileges > and trying to separate them is going to be more harmful than helpful. The > limited things that role attributes permit, and how they fall outside the > privilege/permission concept as we use it, isn't something that I've > noticed is a problem that needs addressing. > > > (I'm slightly >> nervous about the renumbering making the thread hard to follow.) >> >> > 0009 - Something just seems off with this one. Unless there are more > places with this type in use I would just move the relevant notes (i.e., > the one in proallargtypes) to that column and be done with it. If there > are multiple places then moving the notes to the main docs and > cross-referencing to them seems warranted. I also wouldn't call it legacy. > > 0010 - > > When creating new objects, if a schema qualification is not given with the > name the first extant entry in the search_path is chosen; then an error > will be raised should the supplied name already exist in that schema. > In contexts where the object must already exist, but its name is not > schema qualified, the extant search_path schemas will be consulted serially > until one of them contains an appropriate object, returning it, or all > schemas are consulted, resulting in an object not found error. > > I'm not seeing much value in presenting the additional user/public details > here. Especially as it would then seem appropriate to include pg_temp. > And now we have to deal with the fact that by default the public schema > isn't so public anymore. > > 0011 - (first pass, going from memory, might have missed some needed details) Aside from non-atomic SQL routine bodies (functions and procedures) the result of the server executing SQL sent by the connected client does not result in raw SQL, or textual expressions, being stored for later evaluation. All objects are identified (or created) during execution and their effects stored within the system catalogs and assigned system identifiers (oids) to provide an absolute and immutable reference to be used while establishing inter-object dependencies. In short, indirect actions taken by the server, based upon stored knowledge, can and often will execute while in a search_path that only contains the pg_catalog schema so that the stored knowledge can be found. For routines written in any language except Atomic SQL the textual body of the routine is stored as-is within the database. When executing such a routine the (parent) session basically opens up a new connection to the server (one per routine) and within that new sub-session sends the SQL contained within the routine to the server for execution just like any other client, and therefore any object references present in that SQL need to be resolved to a schema as previously discussed. By default, upon connecting, the newly created session is updated so that its settings take on the current values in the parent session. When authoring a routine this is often undesirable as the behavior of the routine no
Re: Various small doc improvements; plpgsql, schemas, permissions, oidvector
On Tue, Oct 3, 2023 at 4:15 PM Karl O. Pinc wrote: > On Tue, 3 Oct 2023 14:51:31 -0700 > "David G. Johnston" wrote: > > Isn't the entire section about "deviating from the normal flow of the > code"? That's what makes me want "Exception" in the section title. > It is about how error handling in a procedure diverts the flow from the normal code path to some other code path - what that path is labelled is less important than the thing that causes the diversion - an error. > ? I remain (overly?) focused on the word "exception", since that's > whats in the brain of the user that's writing RAISE EXCEPTION. > It matters if exceptions and errors are different. If they're > not, then it also matters since it's exceptions that the user's > code raises. > > It's unfortunate the keyword to raise the message level "ERROR" is "EXCEPTION" in that command but I'd rather simply handle that one anomaly that make the rest of the system use the word exception, especially seem to be fairly consistent in our usage of ERROR already. I'm sympathetic that other systems out there also encourage the usage of exception in this context instead of error - but not to the point of opening up this long-standing decision for rework. > Have you any thoughts on the "permissions", "privleges" and > "attributes" vocabulary/concepts used in this area? > I think we benefit from being able to equate permissions and privileges and trying to separate them is going to be more harmful than helpful. The limited things that role attributes permit, and how they fall outside the privilege/permission concept as we use it, isn't something that I've noticed is a problem that needs addressing. (I'm slightly > nervous about the renumbering making the thread hard to follow.) > > 0009 - Something just seems off with this one. Unless there are more places with this type in use I would just move the relevant notes (i.e., the one in proallargtypes) to that column and be done with it. If there are multiple places then moving the notes to the main docs and cross-referencing to them seems warranted. I also wouldn't call it legacy. 0010 - When creating new objects, if a schema qualification is not given with the name the first extant entry in the search_path is chosen; then an error will be raised should the supplied name already exist in that schema. In contexts where the object must already exist, but its name is not schema qualified, the extant search_path schemas will be consulted serially until one of them contains an appropriate object, returning it, or all schemas are consulted, resulting in an object not found error. I'm not seeing much value in presenting the additional user/public details here. Especially as it would then seem appropriate to include pg_temp. And now we have to deal with the fact that by default the public schema isn't so public anymore. David J.
Re: Various small doc improvements; plpgsql, schemas, permissions, oidvector
On Tue, Oct 3, 2023 at 10:56 AM Karl O. Pinc wrote:
> On Mon, 2 Oct 2023 15:18:32 -0500
> "Karl O. Pinc" wrote:
>
> Version 7
>
>
0001 - I would just call the section:
Capturing Command Results into Variables
I would add commentary in there that it is only possible for variables to
take on single value at any given time and so in order to handle multiple
row results you need to instantiate a loop as per 43.6.6
0002 - {Inferred | Indirect} Types ?
We are already in the Declarations section so the fact we are declaring new
variables is already covered.
"Instead of literally writing a type name you can write variable%TYPE and
the system will indirectly apply the then-current type of the named
variable to the newly declared variable." (using "copy the then-current"
reads pretty well and makes the existing title usable...)
0003 - The noun "Exception" here means "deviating from the normal flow of
the code", not, "A subclass of error". I don't see this title change being
particularly beneficial.
0004 - Agreed, but "You can raise an error explicitly as described in
"Errors and Messages". I would not use the phrase "raise an exception", it
doesn't fit.
0005 - This tone of voice is used throughout the introductory documentation
sections, this single change doesn't seem warranted.
0006 - Useful. The view itself provides all configuration parameters known
to the system, the "or selected" isn't true of the view itself, and it
seems to go without saying that the user can add a where clause to any
query they write using that view. At most I'd make one of the examples
include a where clause.
0007 - I'm leaning toward this area being due for some improvement,
especially given the v16 changes bring new attention to it, but this patch
doesn't scream "improvement" to me.
0008 - Same as 0007. INHERIT is no longer an attribute though, it is a
property of membership. This seems more acceptable on its own but I'd need
more time to take in the big picture myself.
That's it for now. I'll look over the other 8 when I can.
David J.
Re: CHECK Constraint Deferrable
On Monday, October 2, 2023, Andreas Joseph Krogh wrote: > På fredag 07. juli 2023 kl. 13:50:44, skrev Dilip Kumar < > dilipbal...@gmail.com>: > > On Wed, Jul 5, 2023 at 3:08 PM Himanshu Upadhyaya > wrote: > > > > Hi, > > > > Currently, there is no support for CHECK constraint DEFERRABLE in a > create table statement. > > SQL standard specifies that CHECK constraint can be defined as > DEFERRABLE. > > I think this is a valid argument that this is part of SQL standard so > it would be good addition to PostgreSQL. So +1 for the feature. > > But I am wondering whether there are some real-world use cases for > deferred CHECK/NOT NULL constraints? I mean like for foreign key > constraints if there is a cyclic dependency between two tables then > deferring the constraint is the simplest way to insert without error. > > > > The real-world use case, at least for me, is when using an ORM. For large > object-graphs ORMs have a tendency to INSERT first with NULLs then UPDATE > the “NOT NULLs” later. > > “Rewrite the ORM” is not an option for most of us… > Between this and Vik comment it sounds like we should probably require a patch in this area to solve both the not null and check constraint deferral omissions then, not just one of them (alternatively, let’s solve the not null one first). David J.
Re: CHECK Constraint Deferrable
On Mon, Oct 2, 2023 at 12:25 PM Tom Lane wrote: > Himanshu Upadhyaya writes: > > V3 patch attached. > > Sorry for not weighing in on this before, but ... is this a feature > we want at all? We are very clear in the existing docs that CHECK > conditions must be immutable [1], and that's not something we can > easily relax because if they are not then it's unclear when we need > to recheck them to ensure they stay satisfied. Agreed. I'm not sold on conforming to the standard being an appropriate ideal here. Either we already don't because our check constraints are immutable, or I'm missing what use case the committee had in mind when they designed this feature. In any case, its absence doesn't seem that sorely missed, and the OP's only actual example would require relaxing the immutable property which I disagree with. We have deferrable triggers to serve that posited use case. David J.
Re: Skip Orderby Execution for Materialized Views
On Sun, Oct 1, 2023 at 8:57 AM Zhang Mingli wrote: > And if it’s true, shall we skip the order by clause for Materialized > View when executing create/refresh statement? > We tend to do precisely what the user writes into their query. If they don't want an order by they can remove it. I don't see any particular reason we should be second-guessing them here. And what makes the trade-off worse is the reasonable expectation that we'd provide a way to force an ordering of the inserts should the user actually want it after we defaulted to ignoring that part of their query. But yes, you are correct that adding an order by to a materialized view is typically pointless. To the extent it is detrimental varies since even partially ordered results can save on processing time. David J.
Re: [DOCS] HOT - correct claim about indexes not referencing old line pointers
On Fri, Sep 29, 2023 at 10:45 AM James Coleman wrote: > Hello, > > While working on my talk for PGConf.NYC next week I came across this > bullet in the docs on heap only tuples: > > > Old versions of updated rows can be completely removed during normal > > operation, including SELECTs, instead of requiring periodic vacuum > > operations. (This is possible because indexes do not reference their page > > item identifiers.) > > But when a HOT update happens the entry in an (logically unchanged) > index still points to the original heap tid, and that line item is > updated with a pointer to the new line pointer in the same page. > > Assuming I'm understanding this correctly, attached is a patch > correcting the description. > > I think we want to somehow distinguish between the old tuple that is the root of the chain and old tuples that are not. This comment refers to pruning the chain and removing intermediate links in the chain that are no longer relevant because the root has been updated to point to the live tuple. In README.HOT, tuple 2 in the example after 1 points to 3. https://github.com/postgres/postgres/blob/ec99d6e9c87a8ff0f4805cc0c6c12cbb89c48e06/src/backend/access/heap/README.HOT#L109 David J.
Re: Set enable_seqscan doesn't take effect?
On Wednesday, September 27, 2023, jacktby jacktby wrote: > postgres=# SET enable_seqscan = off; > SET > postgres=# explain select * from t; >QUERY PLAN > - > Seq Scan on t (cost=100.00..123.60 rows=1360 width=32) It wouldn’t cost 10billion to return the first tuple if that setting wasn’t working. That is the “discouragement” the documentation is referring to. I do agree the wording in the docs could be improved since it is a bit self-contradictory and unspecific, but it is explicitly clear a plan with sequential scan can still be chosen even with this set to off. David J.
Re: to_regtype() Raises Error
On Sunday, September 17, 2023, Chapman Flack wrote:
>
> In this one, both identifiers are part of the type name, and the
> separator a little more flamboyant.
>
> select to_regtype('character /* hi!
> am I part of the type name? /* what, me too? */ ok! */ -- huh!
> varying');
> to_regtype
> ---
> character varying
>
So, maybe we should be saying:
Parses a string of text, extracts a potential type name from it, and
translates that name into an OID. Failure to extract a valid potential
type name results in an error while a failure to determine that the
extracted name is known to the system results in a null output.
I take specific exception to describing your example as a “textual type
name”.
David J.
Re: to_regtype() Raises Error
On Sun, Sep 17, 2023 at 6:25 PM Chapman Flack wrote:
> On 2023-09-17 20:58, David G. Johnston wrote:
> > Put differently, there is no syntax involved when the value being
> > provided
> > is the text literal name of a type as it is stored in pg_type.typname,
> > so
> > the presence of a syntax error is wrong.
>
> Well, the situation is a little weirder than that, because of the
> existence
> of SQL standard types with multiple-token names; when you provide the
> value 'character varying', you are not providing a name found in
> pg_type.typname (while, if you call the same type 'varchar', you are).
> For 'character varying', the parser is necessarily involved.
>
Why don't we just populate pg_type with these standard mandated names too?
>
> The case with 'interval second' is similar, but even different still;
> that isn't a multiple-token type name, but a type name with a
> standard-specified bespoke way of writing a typmod. Another place
> the parser is necessarily involved, doing another job. (AFAICT,
> to_regtype is happy with a typmod attached to the input, and
> happily ignores it, so to_regtype('interval second') gives you
> interval, to_regtype('character varying(20)') gives you
> character varying, and so on.)
>
>
Seems doable to teach the lookup code that suffixes of the form (n) should
be ignored when matching the base type, plus maybe some kind of special
case for standard mandated typmods on their specific types. There is some
ambiguity possible when doing that though:
create type "interval second" as (x int, y int);
select to_regtype('interval second'); --> interval
Or just write a function that deals with the known forms dictated by the
standard and delegate checking for valid names against that first before
consulting pg_type? It might be some code duplication but it isn't like
it's a quickly moving target and I have to imagine it would be faster and
allow us to readily implement the soft-error contract.
David J.
Re: to_regtype() Raises Error
On Sun, Sep 17, 2023 at 5:34 PM Erik Wienhold wrote:
> On 18/09/2023 00:57 CEST Vik Fearing wrote:
>
> > On 9/18/23 00:41, Erik Wienhold wrote:
> > > On 18/09/2023 00:13 CEST David E. Wheeler
> wrote:
> > >
> > >> david=# select to_regtype('inteval second');
> > >> ERROR: syntax error at or near "second"
> > >> LINE 1: select to_regtype('inteval second');
> > >> ^
> > >> CONTEXT: invalid type name "inteval second”
> > >
> > > Probably a typo and you meant 'interval second' which works.
> >
> > No, that is precisely the point. The result should be null instead of
> > an error.
>
> Well, the docs say "return NULL rather than throwing an error if the name
> is
> not found".
> To me "name is not found" implies that it has to be valid syntax
> first to even have a name that can be looked up.
>
Except there is nothing in the typed literal value that is actually a
syntactical problem from the perspective of the user. IOW, the following
work just fine:
select to_regtype('character varying'), to_regtype('interval second');
No need for quotes and the space doesn't produce an issue (and in fact
adding double quotes to the above causes them to not match since the
quoting is taken literally and not syntactically)
The failure to return NULL exposes an implementation detail that we
shouldn't be exposing. As Tom said, maybe doing better is too hard to be
worthwhile, but that doesn't mean our current behavior is somehow correct.
Put differently, there is no syntax involved when the value being provided
is the text literal name of a type as it is stored in pg_type.typname, so
the presence of a syntax error is wrong.
David J.
Re: Can a role have indirect ADMIN OPTION on another role?
On Wed, Sep 6, 2023 at 1:55 PM Ashutosh Sharma wrote: > But what if roleB doesn't want to give roleA access to > the certain objects it owns. Not doable - roleA can always pretend they are roleB one way or another since roleA made roleB. David J.
Re: information_schema and not-null constraints
On Tue, Sep 5, 2023 at 2:50 PM Vik Fearing wrote: > On 9/5/23 19:15, Alvaro Herrera wrote: > > On 2023-Sep-05, Alvaro Herrera wrote: > > > > Looking now at what to do for CHECK_CONSTRAINTS with domain constraints, > > I admit I'm completely confused about what this view is supposed to > > show. Currently, we show the constraint name and a definition like > > "CHECK (column IS NOT NULL)". But since the table name is not given, it > > is not possible to know to what table the column name refers to. For > > domains, we could show "CHECK (VALUE IS NOT NULL)" but again with no > > indication of what domain it applies to, or anything at all that would > > make this useful in any way whatsoever. > > Constraint names are supposed to be unique per schema[1] so the view > contains the minimum required information to identify the constraint. > I'm presuming that the view constraint_column_usage [1] is an integral part of all this though I haven't taken the time to figure out exactly how we are implementing it today. I'm not all that for either A or B since the status quo seems workable. Though ideally if the system has unique names per schema then everything should just work - having the views produce duplicated information (as opposed to nothing) if they are used when the DBA doesn't enforce the standard's requirements seems plausible. David J. [1] https://www.postgresql.org/docs/current/infoschema-constraint-column-usage.html
Re: How to add a new pg oid?
On Tue, Sep 5, 2023, 11:17 jacktby jacktby wrote: > > > 2023年9月5日 22:29，jacktby jacktby 写道： > > > I’m trying to add a new data type for my pg. How to do that? Can you give > me more details or an example > Use create type and let the system deal with it. Otherwise, no, I don't have that knowledge. David J. >
Re: How to add a new pg oid?
OIDs don't exist independently of the data they are associated with. Give more context if you want a better answer. Or just go look at the source code commits for when the last time something needing an OID got added to the core catalog. David J.
Re: PSQL error: total cell count of XXX exceeded
On Friday, August 25, 2023, Hongxu Ma wrote: > > > When I tried to select a big amount of rows, psql complains a error "Cannot > add cell to table content: total cell count of 905032704 exceeded." > > We should use long for ncolumns and nrows and give a more obvious error > message here. > > Any thoughts? or some other hidden reasons? > 9 millions cells seems more than realistic a limit for a psql query result output. In any case it isn’t a bug, the code demonstrates that fact by producing an explicit error. I wouldn’t be adverse to an improved error message, and possibly documenting said limit. David J.
Re: Documentation of psql's \df no longer matches reality
On Thu, Mar 2, 2023 at 3:34 PM Tom Lane wrote: > It seems like we should either restore "trigger" as its own > type classification, or remove it from the list of properties > you can filter on, or adjust the docs to describe "t" as a > special filter condition. I'm kind of inclined to the second > option, because treating trigger as a different prokind sure > seems like a wart. But back in 2009 people thought that was > a good idea; what is our opinion now? > > Personally, I'd go for option 1, bring back the formal concept of a trigger function to this view. Admit the mistake and back-patch so we are consistent again. Or, to improve things, " \df func_name - trigger " should be made to provide a pattern filter on the output type, in which case people could then filter on any type they want, not just trigger. Incorporating set-returning functions into such a filtering mechanism would be a bonus worth striving for. Between choices 2 and 3 above I'd go with 3 before 2. I can imagine the change to label the output of \dft as "func" would easily go unnoticed but removing the existing filtering feature seems likely to draw valid complaints. If we had the more powerful alternative described above to replace it with maybe I'd go for 2. Absent that it is a special case wart necessitated by the lack of being able to readily specify the return type filter in a manner similar to the existing input type filtering. David J.
Re: pgsql: Fix search_path to a safe value during maintenance operations.
On Tue, Aug 1, 2023 at 2:38 PM Jeff Davis wrote: > On Tue, 2023-08-01 at 11:16 -0700, David G. Johnston wrote: > > They can use ALTER FUNCTION and the existing "FROM CURRENT" > > specification to get back to current behavior if desired. > > The current behavior is that the search_path comes from the environment > each execution. FROM CURRENT saves the search_path at definition time > and uses that each execution. > > Right...I apparently misread "create" as "the" in "when CREATE FUNCTION is executed". The overall point stands, it just requires defining a similar "FROM SESSION" to allow for explicitly specifying the current default (missing) behavior. David J.
Re: pgsql: Fix search_path to a safe value during maintenance operations.
On Tue, Aug 1, 2023 at 10:42 AM Robert Haas wrote: > Now, if we don't go in the direction of resolving everything at parse > time, then I think capturing search_path is probably the next best > thing, or at least the next best thing that I've thought up so far. I'd much rather strongly encourage the user to write a safe and self-sufficient function definition. Specifically, if there is no search_path attached to the function then the search path that will be in place will be temp + pg_catalog only. Though I wonder whether it would be advantageous to allow a function to have a temporary schema separate from the session-scoped one... They can use ALTER FUNCTION and the existing "FROM CURRENT" specification to get back to current behavior if desired. Going further, I could envision an "explicit" mode that both performs a parse-time check for object existence and optionally reports an error if the lookup happened via an inexact match - forcing lots more type casts to occur (we'd probably need to invent something to say "must match the anyelement function signature") but ensuring at parse time you've correctly identified everything you intend to be using. Sure, the meanings of those things could change but the surface is much smaller than plugging a new function that matches earlier in the lookup resolution process. David J.
Re: psql: Add role's membership options to the \du+ command
On Wed, Jul 19, 2023 at 9:47 AM Tom Lane wrote: > I wrote: > > Alvaro Herrera writes: > >> +1 for backpatching to 16, given that it's a psql-only change that > >> pertains to a backend change that was done in the 16 timeframe. > > > Agreed. In the interests of moving things along, I'll take point > > on getting this committed. > > And done, with some minor editorialization. I'll go mark the > open item as closed. > > Thank You! David J.
Re: Regarding Installation of PostgreSQL
You are still in the wrong place - this is a developers list, which is only slightly less bad than sending it to a security list. We have a "general" list if you really cannot find a better place to send stuff. But in this case your complaint has to do with the pgAdmin program so its support list would be most appropriate. https://www.postgresql.org/list/pgadmin-support/ That all said, you probably should try downgrading pgAdmin, the 7.4 release seems to be buggy from other reports. David J. On Tue, Jul 18, 2023 at 9:45 AM Sahil Sojitra wrote: > > -- Forwarded message - > From: Sahil Sojitra > Date: Tue, 18 Jul, 2023, 8:43 am > Subject: Regarding Installation of PostgreSQL > To: > > > Hello Sir, >I got stuck into an error repeatedly while installing > PostgreSQL v15.3 and I don't know what to do, while opening pgAdmin 4 just > getting *a blank page* written *Loading pgAdmin 4 v7.4 * plz provide > me the steps to resolve this issue. i am attaching the screenshot of the > error below > >
Re: CommandStatus from insert returning when using a portal.
On Wed, Jul 12, 2023 at 2:49 PM Tom Lane wrote: > Dave Cramer writes: > > 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 > > Umm ... you do realize that we return the rows on-the-fly? > The server does not know how many rows got inserted/returned > until it's run the query to completion, at which point all > the data has already been sent to the client > Doesn't this code contradict that statement? src/backend/tcop/pquery.c /* * If we have not yet run the command, do so, storing its * results in the portal's tuplestore. But we don't do that * for the PORTAL_ONE_SELECT case. */ if (portal->strategy != PORTAL_ONE_SELECT && !portal->holdStore) FillPortalStore(portal, isTopLevel); /* * Now fetch desired portion of results. */ nprocessed = PortalRunSelect(portal, true, count, dest); Not sure we'd want to lock ourselves into this implementation but at least as it stands now we could send a message with the portal size after calling FillPortalStore and prior to calling PortalRunSelect. David J.
Re: CommandStatus from insert returning when using a portal.
On Fri, Jul 14, 2023 at 3:12 PM Chapman Flack wrote: > If someone really does want to do a huge INSERT and get the generated > values back in increments, it might be clearer to write an explicit > INSERT RETURNING and issue it with executeQuery, where everything will > work as expected. > > For PostgreSQL this is even moreso (i.e, huge means count > 1) since the order of rows in the returning clause is not promised to be related to the order of the rows as seen in the supplied insert command. A manual insert returning should ask for not only any auto-generated column but also the set of columns that provide the unique natural key. David J.
Re: CommandStatus from insert returning when using a portal.
On Fri, Jul 14, 2023 at 12:51 PM Tom Lane wrote:
> "David G. Johnston" writes:
> > I agree that the documented contract of the insert command tag says it
> > reports the size of the entire tuple store maintained by the server
> during
> > the transaction instead of just the most recent count on subsequent
> fetches.
>
> Where do you see that documented, exactly? I looked in the protocol
> chapter and didn't find anything definite either way.
>
On successful completion, an INSERT command returns a command tag of the
form
INSERT oid count
The count is the number of rows inserted or updated.
https://www.postgresql.org/docs/current/sql-insert.html
It doesn't, nor should, have any qualifications about not applying to the
returning case and definitely shouldn't change based upon use of FETCH on
the unnamed portal.
I'm quite prepared to believe there are bugs here, since this whole
> set of behaviors is unreachable via libpq: you can't get it to send
> an Execute with a count other than zero (ie, fetch all), nor is it
> prepared to deal with the PortalSuspended messages it'd get if it did.
>
> I think that the behavior is arising from this bit in PortalRun:
>
> if (qc && portal->qc.commandTag != CMDTAG_UNKNOWN)
> {
> CopyQueryCompletion(qc, &portal->qc);
> -->>> qc->nprocessed = nprocessed;
> }
>
I came to the same conclusion. The original introduction of that line
replaced string munging "SELECT " + nprocessed; so this code never even
considered INSERT as being in scope and indeed wanted to return the per-run
value in a fetch-list context.
https://github.com/postgres/postgres/commit/2f9661311b83dc481fc19f6e3bda015392010a40#diff-f66f9adc3dfc98f2ede2e96691843b75128689a8cb9b79ae68d53dc749c3b22bL781
I think I see why simple removal of that line is sufficient as the
copied-from &portal->qc already has the result of executing the underlying
insert query. That said, the paranoid approach would seem to be to keep
the assignment but only use it when we aren't dealing with the returning
case.
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 5565f200c3..5e75141f0b 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -775,7 +775,8 @@ PortalRun(Portal portal, long count, bool isTopLevel,
bool run_once,
if (qc && portal->qc.commandTag !=
CMDTAG_UNKNOWN)
{
CopyQueryCompletion(qc,
&portal->qc);
- qc->nprocessed = nprocessed;
+ if (portal->strategy !=
PORTAL_ONE_RETURNING)
+ qc->nprocessed = nprocessed;
}
/* Mark portal not active */
> It seems plausible to me that we should just remove that marked line.
> Not sure about the compatibility implications, though.
>
>
I believe it is a bug worth fixing, save driver writers processing time
getting a count when the command tag is supposed to be providing it to them
using compute time already spent anyways.
David J.
Re: CommandStatus from insert returning when using a portal.
On Fri, Jul 14, 2023 at 11:34 AM wrote: > On 2023-07-12 21:30, David G. Johnston wrote: > > Right, and executeUpdate is the wrong API method to use, in the > > PostgreSQL > > world, when executing insert/update/delete with the non-SQL-standard > > returning clause. ... ISTM that you are trying to make user-error less > > painful. > > In Dave's Java reproducer, no user-error has been made, because the user > supplied a plain INSERT with the RETURN_GENERATED_KEYS option, and the > RETURNING clause has been added by the JDBC driver. So the user expects > executeUpdate to be the right method, and return the row count, and > getGeneratedKeys() to then return the rows. > > That makes more sense, though I don't understand how the original desire of having the count appear before the actual rows would materially benefit that feature. I agree that the documented contract of the insert command tag says it reports the size of the entire tuple store maintained by the server during the transaction instead of just the most recent count on subsequent fetches. David J.
Re: CommandStatus from insert returning when using a portal.
On Fri, Jul 14, 2023 at 10:39 AM wrote: > On 2023-07-14 12:58, Dave Cramer wrote: > > See attached pcap file > > So if the fetch count is zero and no portal is needed, > or if the fetch count exceeds the row count and the command > completion follows directly with no suspension of the portal, then > it comes with the correct count, but if the portal gets suspended, > then the later command completion reports a zero count? > > I cannot really understand that output other than to confirm that all queries had returning and one of them showed INSERT 0 0 Is there some magic set of arguments I should be using besides: tcpdump -Ar filename ? Because of the returning they all need a portal so far as the server is concerned and the server will obligingly send the contents of the portal back to the client. I can definitely believe a bug exists in the intersection of a non-SELECT query and a less-than-complete fetch count specification. There doesn't seem to be any place in the core testing framework to actually test out the interaction though...I even tried using plpgsql, which lets me open/execute a plpgsql cursor with insert returning (which SQL prohibits) but we can't get access to the command tag itself there. The ROW_COUNT variable likely tracks actual rows fetched or seen (in the case of MOVE). What I kinda suspect might be happening with a portal suspend is that the suspension loop only ends when the final fetch attempt find zero rows to return and thus the final count ends up being zero instead of the cumulative sum over all portal scans. David J.
Re: CommandStatus from insert returning when using a portal.
On Fri, Jul 14, 2023 at 9:50 AM David G. Johnston < david.g.johns...@gmail.com> wrote: > > Fixing that test in some manner and recompiling psql seems like it should > be the easiest way to produce a core-only test case. > > Apparently not - since it (ExecQueryUsingCursor) literally wraps the query in a DECLARE CURSOR SQL Command which prohibits INSERT... I suppose we'd have to write a psql equivalent of ExecQueryUsingPortal that iterates over via fetch to make this work...probably more than I'm willing to try. David J.
Re: CommandStatus from insert returning when using a portal.
On Fri, Jul 14, 2023 at 9:30 AM Dave Cramer wrote:
> David,
>
> I will try to get a tcpdump file. Doing this in libpq seems challenging as
> I'm not aware of how to create a portal in psql.
>
Yeah, apparently psql does something special (like ignoring it...) with its
FETCH_COUNT variable (set to 2 below as evidenced by the first query) for
the insert returning case. As documented since the command itself is not
select or values the test in is_select_command returns false and the branch:
else if (pset.fetch_count <= 0 || pset.gexec_flag ||
pset.crosstab_flag || !is_select_command(query))
{
/* Default fetch-it-all-and-print mode */
Is chosen.
Fixing that test in some manner and recompiling psql seems like it should
be the easiest way to produce a core-only test case.
postgres=# select * from (Values (1),(2),(3),(4)) vals (v);
v
---
1
2
3
4
(4 rows)
postgres=# \bind 5 6 7 8
postgres=# insert into ins values ($1),($2),($3),($4) returning id;
id
---
5
6
7
8
(4 rows)
INSERT 0 4
I was hoping to see the INSERT RETURNING query have a 4 width header
instead of 7.
David J.
Re: CommandStatus from insert returning when using a portal.
On Thu, Jul 13, 2023 at 6:07 PM Dave Cramer wrote: > On Thu, 13 Jul 2023 at 10:24, David G. Johnston < > david.g.johns...@gmail.com> wrote: > >> On Thursday, July 13, 2023, Dave Cramer wrote: >> >>> >>> Any comment on why the CommandComplete is incorrect ? >>> It returns INSERT 0 0 if a cursor is used >>> >> >> Looking at DECLARE it is surprising that what you describe is even >> possible. Can you share a psql reproducer? >> > > apologies, we are using a portal, not a cursor. > > Still the same basic request of providing a reproducer - ideally in psql. IIUC a portal has to be used for a prepared (extended query mode) result set returning query. v16 can now handle parameter binding so: postgres=# \bind 4 postgres=# insert into ins values ($1) returning id; id 4 (1 row) INSERT 0 1 Which gives the expected non-zero command tag row count result. David J.
Re: Fix search_path for all maintenance commands
On Thu, Jul 13, 2023 at 2:00 PM Gurjeet Singh wrote: > 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. > > Here is the instigating issue and a discussion thread on the aftermath: https://wiki.postgresql.org/wiki/A_Guide_to_CVE-2018-1058%3A_Protect_Your_Search_Path https://www.postgresql.org/message-id/flat/13033.1531517020%40sss.pgh.pa.us#2aa2e25816d899d62f168926e3ff17b1 David J.
Re: Fix search_path for all maintenance commands
On Thu, Jul 13, 2023 at 12:54 PM Gurjeet Singh wrote: > > 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. > > If this is limited to MAINT, which I'm in support of, there is no need for an "escape hatch". A prerequisite for leveraging the new feature is that you fix the code so it conforms to the new way of doing things. Tom's opinion was a general dislike for differing behavior in different situations. I dislike it too, on purist grounds, but would rather do this than not make any forward progress because we made a poor decision in the past. And I'm against simply breaking the past without any recourse as what we did for pg_dump/pg_restore still bothers me. David J.
Re: psql: Add role's membership options to the \du+ command
On Thu, Jul 13, 2023 at 8:01 AM Tom Lane wrote:
> > I plan to replace it to:
>
> >pg_catalog.concat_ws(', ',
> > CASE WHEN pam.admin_option THEN 'ADMIN' END,
> > CASE WHEN m.rolinherit THEN 'INHERIT' END,
> > 'SET'
> >) AS "Options",
>
> That does not seem right. Is it impossible for pam.set_option
> to be false? Even if it is, should this code assume that?
>
>
That replacement is for version 15 and earlier where pam.set_option doesn't
exist at all and the presence of a row here means that set has been granted.
David J.
Re: CommandStatus from insert returning when using a portal.
On Thursday, July 13, 2023, Dave Cramer wrote: > > Any comment on why the CommandComplete is incorrect ? > It returns INSERT 0 0 if a cursor is used > Looking at DECLARE it is surprising that what you describe is even possible. Can you share a psql reproducer? David J.
Re: CommandStatus from insert returning when using a portal.
On Wed, Jul 12, 2023 at 5:57 PM Dave Cramer wrote: > On Wed, 12 Jul 2023 at 20:00, wrote: > >> Dave Cramer writes: >> > 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 >> >> Huh ... just how *is* PreparedStatement.execute() supposed >> to behave when the statement is an INSERT RETURNING? >> > > It's really executeUpdate which is supposed to return the number of rows > updated. > Right, and executeUpdate is the wrong API method to use, in the PostgreSQL world, when executing insert/update/delete with the non-SQL-standard returning clause. executeQuery is the method to use. And execute() should behave as if executeQuery was called, i.e., return true, which it is capable of doing since it has resultSet data that it needs to handle. The addition of returning turns the insert/update/delete into a select in terms of effective client-seen behavior. ISTM that you are trying to make user-error less painful. While that is laudable it apparently isn't practical. They can either discard the results and get a count by omitting returning or obtain the result and derive the count by counting rows alongside whatever else they needed the returned data for. David J.
Re: CommandStatus from insert returning when using a portal.
On Wed, Jul 12, 2023 at 2:59 PM Dave Cramer wrote: > On Wed, 12 Jul 2023 at 17:49, Tom Lane wrote: > >> Dave Cramer writes: >> > 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 >> >> Umm ... you do realize that we return the rows on-the-fly? >> > I do realize that. > >> The server does not know how many rows got inserted/returned >> > Well I haven't looked at the code, but it seems unintuitive that adding > the returning clause changes the semantics of insert. > > It doesn't have to - the insertions are always "as rows are produced", it is just that in the non-returning case the final row can be sent to /dev/null instead of the client (IOW, there is always some destination). In both cases the total number of rows inserted are only reliably known when the top executor node requests a new tuple and its immediate predecessor says "no more rows present". David J.
Re: CommandStatus from insert returning when using a portal.
On Wed, Jul 12, 2023 at 1:03 PM Dave Cramer wrote: > > INSERT INTO test_table (cnt) VALUES (1), (2) RETURNING id > > if a portal is used to get the results then the CommandStatus > IIUC the portal is not optional if you including the RETURNING clause. There is no CommandStatus message in the protocol, the desired information is part of the command tag returned in the CommandComplete message. You get that at the end of the command, which has been defined as when any portal produced by the command has been fully executed. You probably should add your desire to the Version 4 protocol ToDo on the wiki. https://wiki.postgresql.org/wiki/Todo#Wire_Protocol_Changes_.2F_v4_Protocol If that ever becomes an active project working through the details of that list for desirability and feasibility would be the first thing to happen. David J.
Re: CHECK Constraint Deferrable
On Friday, July 7, 2023, Himanshu Upadhyaya wrote: > I can think of one scenario, as below > > 1) any department should have an employee > 2)any employee should be assigned to a department > so, the employee table has a FK to the department table, and another check > constraint should be added to the department table to ensure there should > be one/more employees in this department. > > That isn’t a valid/allowed check constraint - it contains a prohibited reference to another table. David J.
Re: psql: Add role's membership options to the \du+ command
On Sat, Jun 24, 2023 at 8:11 AM Pavel Luzanov wrote: > Notes > * The name of the new command. It's a good name, if not for the history. > There are two commands showing the same information about roles: \du and > \dr. > The addition of \drg may be misinterpreted: if there is \drg, then there > is also \dug. > Maybe it's time to think about deprecating of the \du command and leave > only \dg in the next versions? > I would add \dr as the new official command to complement adding \drg and deprecate both \du and \dg. Though actual removal and de-documenting doesn't seem like a good idea. But if we ever did assign something non-role to \dr it would be very confusing. > * The new meta-command will also make sense for versions <16. > The ADMIN OPTION is available in all supported versions. > Doesn't every role pre-16 gain SET permission? We can also deduce whether the grant provides INHERIT based upon the attribute of the role in question. > * The new meta-command will not show all roles. It will only show the > roles included in other roles. > To show all roles you need to add an outer join between pg_roles and > pg_auth_members. > But all columns except "role" will be left blank. Is it worth doing this? > > I'm inclined to want this. I would be good when specifying a role to filter upon that all rows that do exist matching that filter end up in the output regardless if they are standalone or not. David J.
Re: psql: Add role's membership options to the \du+ command
On Fri, Jun 23, 2023 at 5:12 PM Tom Lane wrote: > "David G. Johnston" writes: > > On Thu, Jun 22, 2023 at 5:08 PM Tom Lane wrote: > >> * Personally I could do without the "empty" business, but that seems > >> unnecessary in the tabular format; an empty column will serve fine. > > > I disagree, but not strongly. > > > I kinda expected you to be on the side of "why are we discussing a > > situation that should just be prohibited" though. > > I haven't formed an opinion yet on whether it should be prohibited. > But even if we do that going forward, won't psql need to deal with > such cases when examining old servers? > > I haven't given enough thought to that. My first reaction is that using blank for old servers would be desirable and then, if allowed in v16+ server, "empty" for those. That said, the entire grantor premise that motivated this doesn't exist on those servers so maybe \drg just shouldn't work against pre-v16 servers - and we keep the existing \du query as-is for those as well while removing the "member of" column when \du is executed against a v16+ server. David J.
Re: psql: Add role's membership options to the \du+ command
On Thu, Jun 22, 2023 at 5:08 PM Tom Lane wrote: > "Jonathan S. Katz" writes: > > On 6/15/23 2:47 PM, David G. Johnston wrote: > >> Robert - can you please comment on what you are willing to commit in > >> order to close out your open item here. My take is that the design for > >> this, the tabular form a couple of emails ago (copied here), is > >> ready-to-commit, just needing the actual (trivial) code changes to be > >> made to accomplish it. > > > Can we resolve this before Beta 2?[1] The RMT originally advised to try > > to resolve before Beta 1[2], and this seems to be lingering. > > At this point I kinda doubt that we can get this done before beta2 > either, but I'll put in my two cents anyway: > > * I agree that the "tabular" format looks nicer and has fewer i18n > issues than the other proposals. > As you are on board with a separate command please clarify whether you mean the tabular format but still with newlines, one row per grantee, or the table with one row per grantor-grantee pair. I still like using newlines here even in the separate meta-command. > > * Personally I could do without the "empty" business, but that seems > unnecessary in the tabular format; an empty column will serve fine. > I disagree, but not strongly. I kinda expected you to be on the side of "why are we discussing a situation that should just be prohibited" though. > * I also agree with Pavel's comment that we'd be better off taking > this out of \du altogether and inventing a separate \d command. > Maybe "\drg" for "display role grants"? > Just to be clear, the open item fix proposal is to remove the presently broken (due to it showing duplicates without any context) "member of" array in \du and make a simple table report output in \drg instead. I'm good with \drg as a new meta-command. > * Parenthetically, the "Attributes" column of \du is a complete > disaster > > I hadn't thought about this in detail but did get the same impression. David J.
Re: psql: Add role's membership options to the \du+ command
Robert - can you please comment on what you are willing to commit in order to close out your open item here. My take is that the design for this, the tabular form a couple of emails ago (copied here), is ready-to-commit, just needing the actual (trivial) code changes to be made to accomplish it. Tabular form rolname | memberof | options | grantor --+--+-+-- postgres | | | regress_du_admin | regress_du_role0+| admin, inherit, set+| postgres + | regress_du_role1+| admin, inherit, set+| postgres+ | regress_du_role2 | admin, inherit, set | postgres regress_du_role0 | | | regress_du_role1 | regress_du_role0+| admin, inherit, set+| regress_du_admin+ | regress_du_role0+| inherit +| regress_du_role1+ | regress_du_role0 | set | regress_du_role2 regress_du_role2 | regress_du_role0+| admin +| regress_du_admin+ | regress_du_role0+| inherit, set +| regress_du_role1+ | regress_du_role0+| empty +| regress_du_role2+ | regress_du_role1 | admin, set | regress_du_admin(5 rows) On Thu, May 18, 2023 at 6:07 AM Pavel Luzanov wrote: > On 18.05.2023 05:42, Jonathan S. Katz wrote: > > That said, from a readability standpoint, it was easier for me to follow > the tabular form vs. the sentence form. > > May be possible to reach a agreement on the sentence form. Similar > descriptions used > for referential constraints in the \d command: > > I think we should consider the tabular form with translatable headers to be the acceptable choice here. I don't see enough value in the sentence form to make it worth trying to overcome the i18n objection there. > As for tabular form it looks more natural to have a separate psql command > for pg_auth_members system catalog. Something based on this query:But is it > worth inventing a new psql command for this? > > IMO, no. I'd much rather read "admin, inherit, set" than deal with true/false in columns. I think the newlines are better compared to repetition of the rolname as well. I'm also strongly in favor of explicitly writing out the word "empty" instead of leaving the column blank for the case that no options are marked true. But it isn't a show-stopper for me. David J.
Re: When IMMUTABLE is not.
On Thursday, June 15, 2023, wrote: > > So one could take a strict view that "no PL/Java function should > ever be marked IMMUTABLE" because every one depends on fetching > something (once, at least). > The failure to find and execute the function code itself is not a failure mode that these markers need be concerned with. Assuming one can execute the function an immutable function will give the same answer for the same input for all time. David J.
Re: QUAL Pushdown causes ERROR on syntactically and semantically correct SQL Query
On Mon, Jun 5, 2023, 07:40 Hans Buschmann wrote: > I have reworked the case of BUG #17842 to include the data and the > questions for further investigation. > > > The problem is NOT to correct the query to a working case, but to show a > fundamental problem with qual pushdown. > The optimization system operates with imperfect information, meaning it assumes expressions do not produce errors depending on the data. If you know certain data can produce errors you need to add the relevant code to avoid evaluating those expressions on those data. Yes, or would nice if PostgreSQL could do better here. The cost of doing so is quite high though, and there is no interest in incurring that cost. David J.
Re: Is NEW.ctid usable as table_tuple_satisfies_snapshot?
On Fri, May 26, 2023 at 8:04 AM Kaiting Chen wrote: > I need to implement a trigger that will behave similarly to a foreign key > constraint. The trigger itself will be created with: > > CREATE CONSTRAINT TRIGGER ... AFTER INSERT OR UPDATE OF ... ON foo > > I'd like to skip execution of the trigger logic if, by the time that the > trigger > is executed, the NEW row is no longer valid. > To be clear, when using deferrable constraints and insert then subsequently delete a row you wish to return-early in the insert trigger body as if the row had never been inserted in the first place? The table_tuple_satisfies_snapshot() function is obviously unavailable from > PL/pgSQL. Is this a reliable substitute? > If a row is not visible at the time the trigger fires the SELECT will not return it. When the deferred trigger eventually fires the inserted row will no longer exist and SELECT will not return it. The above is not tested; assuming it does indeed behave that way I would expect the behavior to be deterministic given that there are no concurrency issues involved. > > IF NOT EXISTS (SELECT FROM foo WHERE ctid = NEW.ctid) THEN > RETURN NULL; > END IF; > > Specifically: > > 1. Is there any possibility that, by the time the trigger function is > called, >the NEW row's ctid no longer refers to the row version in NEW, but to an >entirely different row? For example, is it possible for VACUUM to > reclaim the >space at that page number and offset in between the INSERT/UPDATE and > when >the trigger function is called? > No. Transaction and MVCC semantics prevent that from happening. > 2. If I lookup the row by its ctid, will the visibility map be consulted. > No, but that doesn't seem to be material anyway. Your user-space pl/pgsql function shouldn't care about such a purely performance optimization. David J.
Re: Adding SHOW CREATE TABLE
On Sat, May 20, 2023 at 10:26 AM Stephen Frost wrote: > > A server function can be conveniently called from any client code. > > Clearly any client using libpq can conveniently call code which is in > libpq. > > Clearly there are clients that don't use libpq. JDBC comes to mind. David J.
Re: Remove duplicates of membership from results of \du
On Sat, May 6, 2023 at 6:37 AM Shinya Kato wrote: > Hi, hackers > > When executing \du, you can see duplicates of the same role in 'member of'. > This happens when admin | inherit | set options are granted by another > role. > There is already an ongoing patch discussing the needed changes to psql \du because of this change in tracking membership grant attributes. https://www.postgresql.org/message-id/flat/b9be2d0e-a9bc-0a30-492f-a4f68e4f7740%40postgrespro.ru David J.
Re: psql: Add role's membership options to the \du+ command
On Wed, May 3, 2023 at 9:30 AM Jonathan S. Katz wrote: > [Personal hat] > > Looking at Pavel's latest patch, I do find the output easy to > understand, though do we need to explicitly list "empty" if there are no > membership permissions? > > Yes. I dislike having the equivalent of null embedded within the output here. I would rather label it for what it is. As a membership without any attributes has no real purpose I don't see how the choice matters and at least empty both stands out visually and can be grepped. The SQL language uses the words "by" and "from" in its syntax; I'm against avoiding them in our presentation here without a clearly superior alternative that doesn't require a majority of people to have to translate the symbol " / " back into the word " by " in order to read the output. But if it is really a blocker then maybe we should produce 3 separate newline separated columns, one for the member of role, one for the list of attributes, and one with the grantor. The column headers can be translated more easily as single nouns. The readability quite probably would end up being equivalent (maybe even better) in tabular form instead of sentence form. David J.
Re: psql: Add role's membership options to the \du+ command
On Wed, May 3, 2023 at 9:00 AM Jonathan S. Katz wrote: > On 4/13/23 8:44 AM, Pavel Luzanov wrote: > > > P.S. If no objections I plan to add this patch to Open Items for v16 > > https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items > > [RMT hat] > > I don't see why this is an open item as this feature was not committed > for v16. Open items typically revolve around committed features. > > Unless someone makes a convincing argument otherwise, I'll remove this > from the Open Items list[1] tomorrow. > > Thanks, > > Jonathan > > [1] https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items The argument is that updating the psql \d views to show the newly added options is something that the patch to add those options should have done before being committed. Or, at worse, we should decide now that we don't want to do so and spare people the effort of trying to get this committed later. David J.
Re: psql: Add role's membership options to the \du+ command
On Wed, Apr 5, 2023 at 6:58 AM Tom Lane wrote: > Pavel Luzanov writes: > > What if this long output will be available only for \du+, and for \du > > just show distinct (without duplicates) > > roles in the current array format? For those, who don't care about these > > new membership options, nothing will change. > > Those, who need details will use the + modifier. > > ? > > I kind of like that. Would we change to newlines in the Attributes > field in both \du and \du+? (I'm +1 for that, but maybe others aren't.) > > If we don't change the \du "Member of" column display (aside from removing duplicates) I'm disinclined to change the Attributes column. I too am partial to only exposing this detail on the extended (+) display. David J.
Re: psql: Add role's membership options to the \du+ command
On Tue, Apr 4, 2023 at 10:37 AM Tom Lane wrote: > Robert Haas writes: > > On Tue, Apr 4, 2023 at 1:12 PM Tom Lane wrote: > >> I wonder if, while we're here, we should apply the idea of > >> joining-with-newlines-not-commas to the attributes column too. > > > That would make the column narrower, which might be good, because it > > seems to me that listing the memberships could require quite a lot of > > space, both vertical and horizontal. > > Right, that's what I was thinking. > > So, by way of example: regress_du_role1 | cannot login | regress_du_role0 granted by regress_du_admin with admin, inherit, set | Description for regress_du_role1 ~140 character width with description No translations, all words are either identical to syntax or identifiers. I'm on board with newlines in the attributes field. The specific member of column changes are: from -> granted by ( ) -> "with" ais -> admin, inherit, set I'm good with any or all of those selections, either as-is or in the more verbose form. David J.
Re: psql: Add role's membership options to the \du+ command
On Tue, Apr 4, 2023 at 9:13 AM Tom Lane wrote: > "David G. Johnston" writes: > > I've marked this Ready for Committer. > > Hmm ... not sure I like the proposed output. The 'a', 'i', 's' > annotations are short but they don't have much else to recommend them. > On the other hand, there's nearby precedent for single-letter > abbreviations in ACL displays. Nobody particularly likes those, > though. Also, if we're modeling this on ACLs then the display > could be further shortened to "(ais)" or the like. > I am on board with removing the comma and space between the specifiers. My particular issue with the condensed form is readability, especially the lowercase "i". We aren't so desperate for horizontal space here that compaction seems particularly desirable. > > Also, the patch is ignoring i18n issues. Fair point. > I suppose if we stick with > said single-letter abbreviations we'd not translate them, Correct. I don't see this being a huge issue - the abbreviations are the first letter of the various option "keywords" specified in the syntax. > but the > construction "rolename from rolename" ought to be translatable IMO. > Perhaps it'd be enough to allow replacement of "from", but I wonder > if the phrase order would need to be different in some languages? > > Leveraging position and some optional symbols for readability, and sticking with the premise that abbreviations down to the first letter of the relevant syntax keyword is OK: rolename [g: grantor_role] (ais) I don't have any ideas regarding i18n concerns besides avoiding them by not using words...but I'd much prefer "from" and just hope the equivalent in other languages is just as understandable. I'd rather have the above than go and fully try to emulate ACL presentation just to avoid i18n issues. David J.
Re: psql: Add role's membership options to the \du+ command
On Wed, Mar 22, 2023 at 11:11 AM Pavel Luzanov wrote: > In the previous version, I didn't notice (unlike cfbot) the compiler > warning. Fixed in version 6. > > I've marked this Ready for Committer. My opinion is that this is a necessary modification due to the already-committed changes to the membership grant implementation and so only needs to be accepted prior to v16 going live, not feature freeze. I've added Robert to this thread as the committer of said changes. David J.
Re: SELECT INTO without columns or star
On Fri, Mar 31, 2023 at 8:10 AM Zhang Mingli wrote: > When I exec a sql SELECT INTO without columns or * by mistake, it succeeds: > > Yes, a table may have zero columns by design. David J.
Re: Options to rowwise persist result of stable/immutable function with RECORD result
On Wed, Mar 22, 2023 at 4:46 PM David G. Johnston < david.g.johns...@gmail.com> wrote: > On Wed, Mar 22, 2023 at 4:32 PM Eske Rahn wrote: > >> Hi, >> >> Thanks for the quick answer *:-D* >> >> That was a nice sideeffect of lateral. >> >> In the example, the calling code also gets simplified: >> >> WITH x AS ( >> SELECT clock_timestamp() rowstart, *, clock_timestamp() rowend FROM ( >> SELECT '1' inp UNION >> SELECT '2' >> ) y, LATERAL septima.foo(inp) g >> ) >> SELECT * FROM x; >> >> >> That solved the issue at hand, in a much better way. Thanks >> >> Though I still fail to see *why* the other way should generally call the >> function for every column in the *result* record - if the function is >> STABLE or IMMUTABLE. >> > > It gets rewritten to be effectively: > > select func_call(...).col1, func_call(...).col2, func_call(...).col3 > > under the assumption that repeating the function call will be cheap and > side-effect free. It was never ideal but fixing that form of optimization > was harder than implementing LATERAL where the multi-column result has a > natural output in the form of a multi-column table. A normal function call > in the target list really means "return a single value" which is at odds > with writing .* after it. > > Actually, it is less "optimization" and more "SQL is strongly typed and all columns must be defined during query compilation". David J.
Re: Options to rowwise persist result of stable/immutable function with RECORD result
On Wed, Mar 22, 2023 at 4:32 PM Eske Rahn wrote: > Hi, > > Thanks for the quick answer *:-D* > > That was a nice sideeffect of lateral. > > In the example, the calling code also gets simplified: > > WITH x AS ( > SELECT clock_timestamp() rowstart, *, clock_timestamp() rowend FROM ( > SELECT '1' inp UNION > SELECT '2' > ) y, LATERAL septima.foo(inp) g > ) > SELECT * FROM x; > > > That solved the issue at hand, in a much better way. Thanks > > Though I still fail to see *why* the other way should generally call the > function for every column in the *result* record - if the function is > STABLE or IMMUTABLE. > It gets rewritten to be effectively: select func_call(...).col1, func_call(...).col2, func_call(...).col3 under the assumption that repeating the function call will be cheap and side-effect free. It was never ideal but fixing that form of optimization was harder than implementing LATERAL where the multi-column result has a natural output in the form of a multi-column table. A normal function call in the target list really means "return a single value" which is at odds with writing .* after it. David J.
Re: Options to rowwise persist result of stable/immutable function with RECORD result
On Tuesday, March 21, 2023, Eske Rahn wrote: > Hi, > > I have noticed a rather odd behaviour that is not strictly a bug, but is > unexpected. > > It is when a immutable (or stable) PG function is returning results in a > record structure a select on these calls the function repeatedly for each > element in the output record. > The LATERAL join modifier exists to handle this kind of situation. David J.
Re: psql: Add role's membership options to the \du+ command
On Tue, Mar 7, 2023 at 2:02 PM David G. Johnston wrote: > > I'll be looking over your v3 patch sometime this week, if not today. > > Moving the goal posts for this meta-command to >= 9.5 seems like it should be done as a separate patch and thread. The documentation presently states we are targeting 9.2 and newer. My suggestion for the docs is below. I find saying "additional information is shown...currently this adds the comment". Repeating that "+" means (show more) everywhere seems excessive, just state what those "more" things are. I consider \dFp and \dl to be good examples in this regard. I also think that "Wall of text" doesn't serve us well. See \dP for permission to use paragraphs. I didn't modify \du to match; keeping those in sync (as opposed to having \du just say "see \dg") seems acceptable. You had the direction of membership wrong in your copy: "For each membership in the role" describes the reverse of "Member of" which is what the column is. The actual format template is constructed properly. --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -1727,15 +1727,18 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g S modifier to include system roles. If pattern is specified, only those roles whose names match the pattern are listed. -For each membership in the role, the membership options and -the role that granted the membership are displayed. -Оne-letter abbreviations are used for membership options: -a — admin option, i — inherit option, -s — set option and empty if no one is set. -See GRANT command for their meaning. -If the form \dg+ is used, additional information -is shown about each role; currently this adds the comment for each -role. + + +Shown within each row, in newline-separated format, are the memberships granted to +the role. The presentation includes both the name of the grantor +as well as the membership permissions (in an abbreviated format: +a for admin option, i for inherit option, +s for set option.) The word empty is printed in +the case that none of those permissions are granted. +See the GRANT command for their meaning. + + +If the form \dg+ is used the comment attached to the role is shown. I would suggest tweaking the test output to include regress_du_admin and also to make regress_du_admin a CREATEROLE role with LOGIN. I'll need to update the Role Graph View to add the spaces and swap the order of the "s" and "i" symbols. David J.
Re: psql: Add role's membership options to the \du+ command
On Mon, Mar 6, 2023 at 12:43 AM Pavel Luzanov wrote: > Indeed, adding ADMIN to pg_has_role looks logical. The function will show > whether one role can manage another directly or indirectly (via SET ROLE). > FWIW I've finally gotten to publishing my beta version of the Role Graph for PostgreSQL pseudo-extension I'd been talking about: https://github.com/polobo/RoleGraphForPostgreSQL The administration column basically determines all this via a recursive CTE. I'm pondering how to incorporate some of this core material into it now for both cross-validation purposes and ease-of-use. I handle EMPTY explicitly in the Role Graph but as I noted somewhere in my comments, it really shouldn't be possible to leave the database in that state. Do we need to bug Robert on this directly or do you plan to have a go at it? Adding ADMIN will lead to the question of naming other values. It is more > reasonable to have INHERIT instead of USAGE. > And it is not very clear whether (except for backward compatibility) a > separate MEMBER value is needed at all. > There is an appeal to breaking things thoroughly here and removing both MEMBER and USAGE terms while adding ADMIN, SET, INHERIT. However, I am against that. Most everyday usage of this would only likely care about SET and INHERIT even going forward - administration of roles is distinct from how those roles generally behave at runtime. Breaking the later because we improved upon the former doesn't seem defensible. Thus, while adding ADMIN makes sense, keeping MEMBER (a.k.a., SET) and USAGE (a.k.a., INHERIT) is my suggested way forward. I'll be looking over your v3 patch sometime this week, if not today. David J.
Re: NumericShort vs NumericLong format
I'll give this a go as a learning exercise for myself... On Mon, Mar 6, 2023 at 8:47 PM Amin wrote: > > - How can I determine which format will be used for a numeric type? > https://github.com/postgres/postgres/blob/cf96907aadca454c4094819c2ecddee07eafe203/src/backend/utils/adt/numeric.c#L491 (the three constants are decimal 63, 63, and -64 respectively) > - What the precision and scale values should be for pgsql to use the long > format? Is there a threshold? > > Ones that cause the linked-to test to return false I suppose. David J. As an aside, for anyone more fluent than I who reads this, is the use of the word "dynamic scale" in this code comment supposed to be "display scale"? https://github.com/postgres/postgres/blob/cf96907aadca454c4094819c2ecddee07eafe203/src/backend/utils/adt/numeric.c#L121
Re: Request for comment on setting binary format output per session
On Sat, Mar 4, 2023 at 5:07 PM Tom Lane wrote: > Jeff Davis writes: > > On Sat, 2023-03-04 at 18:04 -0500, Dave Cramer wrote: > >> Most of the clients know how to decode the builtin types. I'm not > >> sure there is a use case for binary encode types that the clients > >> don't have a priori knowledge of. > > > The client could, in theory, have a priori knowledge of a non-builtin > > type. > > I don't see what's "in theory" about that. There seems plenty of > use for binary I/O of, say, PostGIS types. Even for built-in types, > do we really want to encourage people to hard-wire their OIDs into > applications? > > I don't see a big problem with driving this off a GUC, but I think > it should be a list of type names not OIDs. We already have plenty > of precedent for dealing with that sort of thing; see search_path > for the canonical example. IIRC, there's similar caching logic > for temp_tablespaces. > > This seems slightly different since types depend upon schemas whereas search_path is top-level and tablespaces are global. But I agree that names should be accepted, maybe in addition to OIDs, the latter, for core types in particular, being a way to not have to worry about masking in user-space. David J.
Re: psql: Add role's membership options to the \du+ command
On Fri, Mar 3, 2023 at 4:01 AM Pavel Luzanov wrote: > Hello, > > On 22.02.2023 00:34, David G. Johnston wrote: > > I didn't even know this function existed. But I see that it was changed in > 3d14e171 with updated documentation: > > https://www.postgresql.org/docs/devel/functions-info.html#FUNCTIONS-INFO-ACCESS > Maybe that's enough. > > > I think that should probably have ADMIN as one of the options as well. > Also curious what it reports for an empty membership. > > > I've been experimenting for a few days and I want to admit that this is a > very difficult and not obvious topic. > I'll try to summarize what I think. > > 1. > About ADMIN value for pg_has_role. > Implementation of ADMIN value will be different from USAGE and SET. > To be True, USAGE value requires the full chain of memberships to have > INHERIT option. > Similar with SET: the full chain of memberships must have SET option. > But for ADMIN, only last member in the chain must have ADMIN option and > all previous members > must have INHERIT (to administer directly) or SET option (to switch to > role, last in the chain). > Therefore, it is not obvious to me that the function needs the ADMIN value. > Or you can SET to some role that then has an unbroken INHERIT chain to the administered role. ADMIN basically implies SET/USAGE but it doesn't work the other way around. I'd be fine with "pg_can_admin_role" being a newly created function that provides this true/false answer but it seems indisputable that today there is no core-provided means to answer the question "can one role get ADMIN rights on another role". Modifying \du to show this seems out-of-scope but the pg_has_role function already provides that question for INHERIT and SET so it is at least plausible to extend it to include ADMIN, even if the phrase "has role" seems a bit of a misnomer. I do cover this aspect with the Role Graph pseudo-extension but given the presence and ease-of-use of a boolean-returning function this seems like a natural addition. We've also survived quite long without it - this isn't a new concept in v16, just a bit refined. > > 2. > pg_has_role function description starts with: Does user have privilege for > role? > - This is not exact: function works not only with users, but with > NOLOGIN roles too. > - Term "privilege": this term used for ACL columns, such usage may be > confusing, > especially after adding INHERIT and SET in addition to ADMIN option. > Yes, it missed the whole "there are only roles now" memo. I don't have an issue with using privilege here though - you have to use the GRANT command which "defines access privileges". Otherwise "membership option" or maybe just "option" would need to be explored. > > 3. > It is possible to grant membership with all three options turned off: > grant a to b with admin false, inherit false, set false; > But such membership is completely useless (if i didn't miss something). > May be such grants must be prohibited. At least this may be documented in > the GRANT command. > I have no issue with prohibiting the "empty membership" if someone wants to code that up. > 4. > Since v16 it is possible to grant membership from one role to another > several times with different grantors. > And only grantor can revoke membership. > - This is not documented anywhere. > Yeah, a pass over the GRANTED BY actual operation versus documentation seems warranted. > - Current behavior of \du command with duplicated roles in "Member of" > column strongly confusing. > This is one of the goals of the discussion patch. > This indeed needs to be fixed, one way (include grantor) or the other (du-duplicate), with the current proposal of including grantor getting my vote. > > I think to write about this to pgsql-docs additionally to this topic. > I wouldn't bother starting yet another thread in this area right now, this one can absorb some related changes as well as the subject line item. David J.
Re: What's the prefix?
On Sun, Feb 26, 2023 at 9:16 AM jack...@gmail.com wrote:
> use these sqls:
> create table t(a text);
> insert into t values('a');
> select lp,lp_len,t_data from heap_page_items(get_raw_page('t',0));
> lp | lp_len | t_data
> ++
> 1 | 26 | \x0561
> as you can see, the 61 is 'a', so what's the 05??? strange.
>
text is variable length so there is header information built into the
datatype representation that indicates how long the content is.
David J.
Re: Re: Give me more details of some bits in infomask!!
On Sun, Feb 26, 2023 at 8:36 AM jack...@gmail.com wrote: > > CID means "command ID" i.e. sequential ID assigned to commands in a > > single session (for visibility checks, so that a query doesn't see data > > deleted by earlier commands in the same session). See > > src/backend/utils/time/combocid.c for basic explanation of what "combo > > CID" is. > I think if cid is used for visibility checks in one session, that's > meaingless, beacause we can use the t_xmin and t_xmax to > get this goal. Is tis > > I think the word "session" is wrong. It should be "transaction". IIUC, it is what is changed when one issues CommandCounterIncrement within a transaction. And you need somewhere to save which CCI step deletes rows, in particular due to the savepoint feature. David J.
Re: Re: Why the lp_len is 28 not 32?
On Sun, Feb 26, 2023 at 8:11 AM jack...@gmail.com wrote: > > *From:* Tomas Vondra > > > +++ > > 1 | 8160 | 28 | \x0100 > > > > > Pretty sure this is because we align the data to MAXALIGN, and on x86_64 > that's 8 bytes. 28 is not a multiple of 8 while 32 is. > > >> yes, So it should be 32 bytes not 28bytes, but the sql result is 28 > !! that's false > > No, that is a definition not matching your expectation. Are you trying to demonstrate a bug here or just observing that your intuition of this didn't work here? The content doesn't include alignment padding. The claim isn't that the size is "the number of bytes consumed in some place within the page" but rather the size is "the number of bytes needed to get the content required to be passed into the input function for the datatype". Nicely, it is trivial to then align the value to figure out the consumed width. If you just have the aligned size you would never know how many bytes you need for the data value. David J.
Re: psql: Add role's membership options to the \du+ command
On Tue, Feb 21, 2023 at 2:14 PM Pavel Luzanov
wrote:
> On 17.02.2023 19:53, David G. Johnston wrote:
>
> On Fri, Feb 17, 2023 at 4:02 AM Pavel Luzanov
> wrote:
>
>>List of roles
>> Role name | Attributes |
>> Member of
>>
>> ---++---
>> admin | Create role|
>> {bob,bob}
>> bob ||
>> {}
>> postgres | Superuser, Create role, Create DB, Replication, Bypass RLS |
>> {}
>>
>> First 'grant bob to admin' command issued immediately after creating role
>> bob by superuser(grantor=10). Second command issues by admin role and set
>> membership options SET and INHERIT.
>> If we don't ready to display membership options with \du+ may be at least
>> we must group records in 'Member of' column for \du command?
>>
>
> I agree that these views should GROUP BY roleid and use bool_or(*_option)
> to produce their result.
>
>
> Ok, I'll try in the next few days. But what presentation format to use?
>
>
This is the format I've gone for (more-or-less) in my RoleGraph view (I'll
be sharing it publicly in the near future).
bob from grantor (a, s, i) \n
adam from postgres (a, s, i) \n
emily from postgres (empty)
I don't think first-letter mnemonics will be an issue - you need to learn
the syntax anyway. And it is already what we do for object grants besides.
Based upon prior comments going for something like the following is
undesirable: bob=asi/grantor
So I converted the "/" into "from" and stuck the permissions on the end
instead of in the middle (makes reading the "from" fragment cleaner).
To be clear, this is going away from grouping but trades verbosity and
deviation from what is done today for better information. If we are going
to break this I suppose we might as well break it thoroughly.
>
> I didn't even know this function existed. But I see that it was changed in
> 3d14e171 with updated documentation:
>
> https://www.postgresql.org/docs/devel/functions-info.html#FUNCTIONS-INFO-ACCESS
> Maybe that's enough.
>
>
I think that should probably have ADMIN as one of the options as well.
Also curious what it reports for an empty membership.
David J.
Re: Improving inferred query column names
On Mon, Feb 20, 2023 at 8:08 AM Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > On 11.02.23 20:24, Andres Freund wrote: > > > > I think on a green field it'd be clearly better to do something like the > > above. What does give me pause is that it seems quite likely to break > > existing queries, and to a lesser degree, might break applications > relying on > > inferred column names > > > > Can anybody think of a good way out of that? It's not like that problem > is > > going to go away at some point... > > I think we should just do it and not care about what breaks. There has > never been any guarantee about these. > > I'm going to toss a -1 into the ring but if this does go through a strong request that it be disabled via a GUC. The ugliness of that option is why we shouldn't do this. Defacto reality is still a reality we are on the hook for. I too find the legacy design choice to be annoying but not so much that changing it seems like a good idea. David J.
Re: psql: Add role's membership options to the \du+ command
On Fri, Feb 17, 2023 at 4:02 AM Pavel Luzanov
wrote:
>List of roles
> Role name | Attributes |
> Member of
>
> ---++---
> admin | Create role|
> {bob,bob}
> bob ||
> {}
> postgres | Superuser, Create role, Create DB, Replication, Bypass RLS |
> {}
>
> First 'grant bob to admin' command issued immediately after creating role
> bob by superuser(grantor=10). Second command issues by admin role and set
> membership options SET and INHERIT.
> If we don't ready to display membership options with \du+ may be at least
> we must group records in 'Member of' column for \du command?
>
I agree that these views should GROUP BY roleid and use bool_or(*_option)
to produce their result. Their purpose is to communicate the current
effective state to the user, not facilitate full inspection of the
configuration, possibly to aid in issuing GRANT and REVOKE commands.
One thing I found, and I plan to bring this up independently once I've
collected my thoughts, is that pg_has_role() uses the terminology "USAGE"
and "MEMBER" for "INHERIT" and "SET" respectively.
It's annoying that "member" has been overloaded here. And the choice of
USAGE just seems arbitrary (though I haven't researched it) given the
related syntax.
https://www.postgresql.org/docs/15/functions-info.html
Re: psql: Add role's membership options to the \du+ command
On Wed, Feb 15, 2023 at 2:31 PM David Zhang wrote: > There is a default built-in role `pg_monitor` and the behavior changed > after the patch. If `\dg+` and `\du+` is treated as the same, and `make > check` all pass, then I assume there is no test case to verify the output > of `duS+`. My point is should we consider add a test case? > I mean, either you accept the change in how this meta-command presents its information or you don't. I don't see how a test case is particularly beneficial. Or, at least the pg_monitor role is not special in this regard. Alice changed too and you don't seem to be including it in your complaint. David J.
Re: psql: Add role's membership options to the \du+ command
On Fri, Feb 10, 2023 at 2:08 PM David Zhang wrote: > > I noticed the document psql-ref.sgml has been updated for both `du+` and > `dg+`, but only `du` and `\du+` are covered in regression test. Is that > because `dg+` is treated exactly the same as `du+` from testing point of > view? > Yes. > > The reason I am asking this question is that I notice that `pg_monitor` > also has the detailed information, so not sure if more test cases required. > Of course it does. Why does that bother you? And what does that have to do with the previous paragraph? David J.
Re: pg_dump versus hash partitioning
On Wed, Feb 1, 2023 at 3:38 PM Tom Lane wrote: > Peter Geoghegan writes: > > You mentioned "minor releases" here. Who said anything about that? > > I did: I'd like to back-patch the fix if possible. I think changing > the default --load-via-partition-root choice could be back-patchable. > > If Robert is resistant to that but would accept it in master, > I'd settle for that in preference to having no fix. > I'm for accepting --load-via-partition-root as the solution to this problem. I think it is better than doing nothing and, at the moment, I don't see any alternatives to pick from. As evidenced from the current influx of collation problems related to indexes, we would be foolish to discount the collation issues with just plain text, so limiting this only to the enum case (which is a must-have) doesn't seem wise. pg_dump should be conservative in what it produces - which in this situation means having minimal environmental dependencies and internal volatility. In the interest of compatibility, having an escape hatch to do things as they are done today is something we should provide. We got this one wrong and that is going to cause some pain. Though at least with the escape hatch we shouldn't be dealing with as many unresolvable complaints as when we back-patched removing the public schema from search_path. In the worst case, being conservative, the user can always at minimum restore their dump file into a local database and get access to their data in a usable format with minimal hassle. The few that would like "same table name or bust" behavior because of external or application-specific requirements can still get that behavior. David J.
