Re: Virtual generated columns

[Corey Huinker]

On Mon, Apr 29, 2024 at 4:24 AM Peter Eisentraut 
wrote:

> Here is a patch set to implement virtual generated columns.
>

I'm very excited about this!



> The main feature patch (0005 here) generally works but has a number of
> open corner cases that need to be thought about and/or fixed, many of
> which are marked in the code or the tests.  I'll continue working on
> that.  But I wanted to see if I can get some feedback on the test
> structure, so I don't have to keep changing it around later.
>

I'd be very interested to see virtual generated columns working, as one of
my past customers had a need to reclassify data in a partitioned table, and
the ability to detach a partition, alter the virtual generated columns, and
re-attach would have been great. In case you care, it was basically an
"expired" flag, but the rules for what data "expired" varied by country of
customer and level of service.

+ * Stored generated columns cannot work: They are computed after
+ * BEFORE triggers, but partition routing is done before all
+ * triggers.  Maybe virtual generated columns could be made to
+ * work, but then they would need to be handled as an expression
+ * below.

I'd say you nailed it with the test structure. The stored/virtual
copy/split is the ideal way to approach this, which makes the diff very
easy to understand.

+1 for not handling domain types yet.

 -- generation expression must be immutable
-CREATE TABLE gtest_err_4 (a int PRIMARY KEY, b double precision GENERATED
ALWAYS AS (random()) STORED);
+CREATE TABLE gtest_err_4 (a int PRIMARY KEY, b double precision GENERATED
ALWAYS AS (random()) VIRTUAL);


Does a VIRTUAL generated column have to be immutable? I can see where the
STORED one has to be, but consider the following:

CREATE TABLE foo (
created_at timestamptz DEFAULT CURRENT_TIMESTAMP,
row_age interval GENERATED ALWAYS AS CURRENT_TIMESTAMP - created_at
);


 -- can't have generated column that is a child of normal column
 CREATE TABLE gtest_normal (a int, b int);
-CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2)
STORED) INHERITS (gtest_normal);  -- error
+CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2)
VIRTUAL) INHERITS (gtest_normal);  -- error


This is the barrier to the partitioning reorganization scheme I described
above. Is there any hard rule why a child table couldn't have a generated
column matching the parent's regular column? I can see where it might
prevent indexing that column on the parent table, but is there some other
dealbreaker or is this just a "it doesn't work yet" situation?

One last thing to keep in mind is that there are two special case
expressions in the spec:

GENERATED ALWAYS AS ROW START
GENERATED ALWAYS AS ROW END


and we'll need to be able to fit those into the catalog. I'll start another
thread for that unless you prefer I keep it here.

Re: documentation structure

[Corey Huinker]

>
> I've splitted it to7 patches.
> each patch split one  into separate new files.
>

Seems like a good start. Looking at the diffs of these, I wonder if we
would be better off with a func/ directory, each function gets its own file
in that dir, and either these files above include the individual files, or
the original func.sgml just becomes the organizer of all the functions.
That would allow us to do future reorganizations with minimal churn, make
validation of this patch a bit more straightforward, and make it easier for
future editors to find the function they need to edit.

Re: documentation structure

[Corey Huinker]

>
> Yeah, we can't expect everyone wanting to call a built-in function to
> know how they would define an equivalent one themselves. In that case I
> propos marking it up like this:
>
> format (
> formatstr text
> , formatarg "any"
> , ...  )
> text
>

Looks good, but I guess I have to ask: is there a parameter-list tag out
there instead of (, and should we be using that?



> The requisite nesting when there are multiple optional parameters makes
> it annoying to wrap and indent it "properly" per XML convention, but how
> about something like this, with each parameter on a line of its own, and
> all the closing  tags on one line?
>
> regexp_substr (
> string text,
> pattern text
> , start integer
> , N integer
> , flags text
> , subexpr integer
> )
> text
>

Yes, that has an easy count-the-vertical, count-the-horizontal,
do-they-match flow to it.


> A lot of functions mostly follow this style, except they tend to put the
> first parameter on the same line of the function namee, even when that
> makes the line overly long. I propose going the other way, with each
> parameter on a line of its own, even if the first one would fit after
> the function name, except the whole parameter list fits after the
> function name.
>

+1


>
> Also, when there's only one optional argument, or they're independently
> optional, not nested, the  tag should go on the same line as
> the parameter.
>
> substring (
> bits bit
>  FROM start
> integer 
>  FOR count
> integer  )
> bit
>

+1

Re: documentation structure

[Corey Huinker]

>
> I havent dealt with variadic yet, since the two styles are visually
> different, not just markup (... renders as [...]).
>
> The two styles for variadic are the what I call caller-style:
>
>concat ( val1 "any" [, val2 "any" [, ...] ] )
>format(formatstr text [, formatarg "any" [, ...] ])
>

While this style is obviously clumsier for us to compose, it does avoid
relying on the user understanding what the word variadic means. Searching
through online documentation of the python *args parameter, the word
variadic never comes up, the closest they get is "variable length
argument". I realize that python is not SQL, but I think it's a good point
of reference for what concepts the average reader is likely to know.

Looking at the patch, I think it is good, though I'd consider doing some
indentation for the nested s to allow the author to do more
visual tag-matching. The ']'s were sufficiently visually distinct that we
didn't really need or want nesting, but  is just another tag to
my eyes in a sea of tags.

Re: improve performance of pg_dump --binary-upgrade

[Corey Huinker]

>
> One downside of this approach is the memory usage.  This was more-or-less
>
>
Bar-napkin math tells me in a worst-case architecture and braindead byte
alignment, we'd burn 64 bytes per struct, so the 100K tables cited would be
about 6.25MB of memory.

The obvious low-memory alternative would be to make a prepared statement,
though that does nothing to cut down on the roundtrips.

I think this is a good trade off.

Re: documentation structure

[Corey Huinker]

>
> And it's very inconsistent.  For example, some functions use 
> tags for optional parameters, others use square brackets, and some use
> VARIADIC to indicate variadic parameters, others use
> ellipses (sometimes in  tags or brackets).


Having just written a couple of those functions, I wasn't able to find any
guidance on how to document them with regards to  vs [], etc.
Having such a thing would be helpful.

While we're throwing out ideas, does it make sense to have function
parameters and return values be things that can accept COMMENTs? Like so:

COMMENT ON FUNCTION function_name [ ( [ [ argmode ] [ argname ] argtype [,
...] ] ) ] ARGUMENT argname IS '';
COMMENT ON FUNCTION function_name [ ( [ [ argmode ] [ argname ] argtype [,
...] ] ) ] RETURN VALUE IS '';

I don't think this is a great idea, but if we're going to auto-generate
documentation then we've got to store the metadata somewhere, and
pg_proc.dat is already lacking relevant details.

Re: documentation structure

[Corey Huinker]

>
> > This sounds to me like it would be a painful exercise with not a
> > lot of benefit in the end.
>
> Maybe we could _verify_ the contents of func.sgml against pg_proc.
>

All of the functions redefined in catalog/system_functions.sql complicate
using pg_proc.dat as a doc generator or source of validation. We'd probably
do better to validate against a live instance, and even then the benefit
wouldn't be great.

Importing Extended Statistics

[Corey Huinker]

I'm creating this new thread separate from the existing Statistics
Export/Import thread to keep the original thread focused on that patch.

Assuming that the function signature for pg_set_attribute_stats() remains
the same (regclass, attname, inherited, version, ...stats...), how would we
design the function signature for pg_set_extended_stats()?

Currently, the variant arguments in pg_set_attribute_stats are mapping
attribute names from pg_stats onto parameter names in the function, so a
call looks like this:

SELECT pg_set_attribute_stats('public.foo'::regclass, 'foo_id', false,
17000,
'null_frac', 0.4::real,
'avg_width', 20::integer,
'n_distinct, ', 100::real,
...);


And that works, because there's a 1:1 mapping of attribute names to param
names in the pairs of variants.

However, that won't work for extended stats, as it has 2 mappings:

* 1 set of stats from pg_stats_ext
* N sets of stats from pg_stats_ext_exprs, one per expression contained in
the statistics object definition.

My first attempt at making this possible is to have section markers. The
variant arguments would be matched against column names in pg_stats_ext
until the parameter 'expression' is encountered, at which point it would
begin matching parameters from pg_stats_ext_exprs to parameters for the
first expression. Any subsequent use of 'expression' would move the
matching to the next expression.

This method places a burden on the caller to get all of the pg_stats_ext
values specified before any expression values. It also places a burden on
the reader that they have to count the number of times they see
'expression' used as a parameter, but it otherwise allows 1 variant list to
serve two purposes.

Thoughts?

Re: Statistics Import and Export

[Corey Huinker]

>
>
>
> I think it'll be a serious, serious error for this not to be
> SECTION_DATA.  Maybe POST_DATA is OK, but even that seems like
> an implementation compromise not "the way it ought to be".
>

We'd have to split them on account of when the underlying object is
created. Index statistics would be SECTION_POST_DATA, and everything else
would be SECTION_DATA. Looking ahead, statistics data for extended
statistics objects would also be POST. That's not a big change, but my
first attempt at that resulted in a bunch of unrelated grants dumping in
the wrong section.

Re: Statistics Import and Export

[Corey Huinker]

>
> For a partitioned table this value has to be true. For a normal table when
> setting this value to true, it should at least make sure that the table has
> at least one child. Otherwise it should throw an error. Blindly accepting
> the given value may render the statistics unusable. Prologue of the
> function needs to be updated accordingly.
>

I can see rejecting non-inherited stats for a partitioned table. The
reverse, however, isn't true, because a table may end up being inherited by
another, so those statistics may be legit. Having said that, a great deal
of the data validation I was doing was seen as unnecessary, so I' not sure
where this check would fall on that line. It's a trivial check if we do add
it.

Re: Statistics Import and Export

[Corey Huinker]

>
>
> As far as that goes, it shouldn't be that hard to deal with, at least
> not for "soft" errors which hopefully cover most input-function
> failures these days.  You should be invoking array_in via
> InputFunctionCallSafe and passing a suitably-set-up ErrorSaveContext.
> (Look at pg_input_error_info() for useful precedent.)
>

Ah, my understanding may be out of date. I was under the impression that
that mechanism relied on the the cooperation of the per-element input
function, so even if we got all the builtin datatypes to play nice with
*Safe(), we were always going to be at risk with a user-defined input
function.


> There might be something to be said for handling all the error
> cases via an ErrorSaveContext and use of ereturn() instead of
> ereport().  Not sure if it's worth the trouble or not.
>

It would help us tailor the user experience. Right now we have several
endgames. To recap:

1. NULL input =>  Return NULL. (because strict).
2. Actual error (permissions, cache lookup not found, etc) => Raise ERROR
(thus ruining binary upgrade)
3. Call values are so bad (examples: attname not found, required stat
missing) that nothing can recover => WARN, return FALSE.
4. At least one stakind-stat is wonky (impossible for datatype, missing
stat pair, wrong type on input parameter), but that's the worst of it => 1
to N WARNs, write stats that do make sense, return TRUE.
5. Hunky-dory. => No warns. Write all stats. return TRUE.

Which of those seem like good ereturn candidates to you?

Re: Statistics Import and Export

[Corey Huinker]

>
> Yeah, but that problem exists no matter what.  I haven't read enough
> of the patch to find where it's determining that, but I assume there's
> code in there to intuit the statistics storage type depending on the
> table column's data type and the statistics kind.
>

Correct. It borrows a lot from examine_attribute() and the *_typanalyze()
functions. Actually using VacAttrStats proved problematic, but that can be
revisited at some point.


> We could not trust the exporting side to tell us the correct answer;
> for one reason, it might be different across different releases.
> So "derive it reliably on the destination" is really the only option.
>

+1


> I think that it's impossible to do this in the general case, since
> type-specific typanalyze functions can store pretty nearly whatever
> they like.  However, the pg_stats view isn't going to show nonstandard
> statistics kinds anyway, so we are going to be lossy for custom
> statistics kinds.
>

Sadly true.

Re: Statistics Import and Export

[Corey Huinker]

>
> side to know the element type anyway.  So, I apologize for sending
> us down a useless side path.  We may as well stick to the function
> signature as shown in the v15 patch --- although maybe variadic
> any is still worthwhile so that an unrecognized field name doesn't
> need to be a hard error?
>

Variadic is nearly done. This issue was the main blocking point. I can go
back to array_in() as we know that code works.

Re: Statistics Import and Export

[Corey Huinker]

I have refactored pg_set_relation_stats to be variadic, and I'm working on
pg_set_attribute_sttats, but I'm encountering an issue with the anyarray
values.

Jeff suggested looking at anyarray_send as a way of extracting the type,
and with some extra twiddling we can get and cast the type. However, some
of the ANYARRAYs have element types that are themselves arrays, and near as
I can tell, such a construct is not expressible in SQL. So, rather than
getting an anyarray of an array type, you instead get an array of one
higher dimension.  Like so:

# select schemaname, tablename, attname,

 substring(substring(anyarray_send(histogram_bounds) from 9 for
4)::text,2)::bit(32)::integer::regtype,


 substring(substring(anyarray_send(histogram_bounds::text::text[][]) from 9
for 4)::text,2)::bit(32)::integer::regtype
from pg_stats where histogram_bounds is not null

and tablename = 'pg_proc' and attname = 'proargnames'


  ;

 schemaname | tablename |   attname   | substring | substring

+---+-+---+---

 pg_catalog | pg_proc   | proargnames | text[]| text

Luckily, passing in such a value would have done all of the element
typechecking for us, so we would just move the data to an array of one less
dimension typed elem[]. If there's an easy way to do that, I don't know of
it.

What remains is just checking the input types against the expected type of
the array, stepping down the dimension if need be, and skipping if the type
doesn't meet expectations.

Re: Statistics Import and Export

[Corey Huinker]

Here's a one-liner patch for disabling update of pg_class
relpages/reltuples/relallviible during a binary upgrade.

This was causting pg_upgrade tests to fail in the existing stats import
work.
From 322db8c9e8796ce673f7d7b63bd64e4c9403a144 Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Mon, 1 Apr 2024 18:25:18 -0400
Subject: [PATCH v1] Disable updating pg_class for CREATE INDEX during binary
 upgrade.

There is no point in setting these values because the table will always
be empty.
---
 src/backend/catalog/index.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index b6a7c60e23..f83b35add5 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2874,7 +2874,7 @@ index_update_stats(Relation rel,
 		dirty = true;
 	}
 
-	if (reltuples >= 0)
+	if ((reltuples >= 0) && (!IsBinaryUpgrade))
 	{
 		BlockNumber relpages = RelationGetNumberOfBlocks(rel);
 		BlockNumber relallvisible;
-- 
2.44.0

Re: Statistics Import and Export

[Corey Huinker]

>
> If we are envisioning that the function might emit multiple warnings
> per call, a useful definition could be to return the number of
> warnings (so zero is good, not-zero is bad).  But I'm not sure that's
> really better than a boolean result.  pg_dump/pg_restore won't notice
> anyway, but perhaps other programs using these functions would care.
>

A boolean is what we had before, I'm quite comfortable with that, and it
addresses my silent-failure concerns.

Re: Statistics Import and Export

[Corey Huinker]

>
>
> I still think that we could just declare the function strict, if we
> use the variadic-any approach.  Passing a null in any position is
> indisputable caller error.  However, if you're allergic to silently
> doing nothing in such a case, we could have pg_set_attribute_stats
> check each argument and throw an error.  (Or warn and keep going;
> but according to the design principle I posited earlier, this'd be
> the sort of thing we don't need to tolerate.)
>

Any thoughts about going back to having a return value, a caller could then
see that the function returned NULL rather than whatever the expected value
was (example: TRUE)?

Re: Statistics Import and Export

[Corey Huinker]

>
> Ah, yeah, you could change the function to have more parameters,
> given the assumption that all calls will be named-parameter style.
> I still suggest that my proposal is more robust for the case where
> the dump lists parameters that the receiving system doesn't have.
>

So what's the behavior when the user fails to supply a parameter that is
currently NOT NULL checked (example: avg_witdth)? Is that a WARN-and-exit?

Re: Statistics Import and Export

[Corey Huinker]

>
> I think pg_upgrade could check for the existence of extended statistics
> in any database and adjust the analyze recommdnation wording
> accordingly.
>

+1

Re: Statistics Import and Export

[Corey Huinker]

>
> That's not what I suggested at all.  The function parameters would
> be declared anyarray, but the values passed to them would be coerced
> to the correct concrete array types.  So as far as the coercion rules
> are concerned this'd be equivalent to the variadic-any approach.
>

+1



>
> > That's pretty persuasive. It also means that we need to trap for error in
> > the array_in() calls, as that function does not yet have a _safe() mode.
>
> Well, the approach I'm advocating for would have the array input and
> coercion done by the calling query before control ever reaches
> pg_set_attribute_stats, so that any incorrect-for-the-data-type values
> would result in hard errors.  I think that's okay for the same reason
> you probably figured you didn't have to trap array_in: it's the fault
> of the originating pg_dump if it offers a value that doesn't coerce to
> the datatype it claims the value is of.


+1

Re: Statistics Import and Export

[Corey Huinker]

>
> IIRC, "variadic any" requires having at least one variadic parameter.
> But that seems fine --- what would be the point, or even the
> semantics, of calling pg_set_attribute_stats with no data fields?
>

If my pg_dump run emitted a bunch of stats that could never be imported,
I'd want to know. With silent failures, I don't.



> Perhaps we could
> invent a new backend function that extracts the actual element type
> of a non-null anyarray argument.
>

A backend function that we can't guarantee exists on the source system. :(


> Another way we could get to no-coercions is to stick with your
> signature but declare the relevant parameters as anyarray instead of
> text.  I still think though that we'd be better off to leave the
> parameter matching to runtime, so that we-don't-recognize-that-field
> can be a warning not an error.
>

I'm a bit confused here. AFAIK we can't construct an anyarray in SQL:

# select '{1,2,3}'::anyarray;
ERROR:  cannot accept a value of type anyarray


> I think you missed my point: you're doing that inefficiently,
> and maybe even with race conditions.  Use the relcache's copy
> of the pg_class row.
>

Roger Wilco.


> Well, I'm here to debate it if you want, but I'll just note that *one*
> error will be enough to abort a pg_upgrade entirely, and most users
> these days get scared by errors during manual dump/restore too.  So we
> had better not be throwing errors except for cases that we don't think
> pg_dump could ever emit.
>

That's pretty persuasive. It also means that we need to trap for error in
the array_in() calls, as that function does not yet have a _safe() mode.

Re: Statistics Import and Export

[Corey Huinker]

>
>
> I concur with the plan of extracting data from pg_stats not
> pg_statistics, and with emitting a single "set statistics"
> call per attribute.  (I think at one point I'd suggested a call
> per stakind slot, but that would lead to a bunch of UPDATEs on
> existing pg_attribute tuples and hence a bunch of dead tuples
> at the end of an import, so it's not the way to go.  A series
> of UPDATEs would likely also play poorly with a background
> auto-ANALYZE happening concurrently.)
>

That was my reasoning as well.



> I do not like the current design for pg_set_attribute_stats' API
> though: I don't think it's at all future-proof.  What happens when
> somebody adds a new stakind (and hence new pg_stats column)?
> You could try to add an overloaded pg_set_attribute_stats
> version with more parameters, but I'm pretty sure that would
> lead to "ambiguous function call" failures when trying to load
> old dump files containing only the original parameters.


I don't think we'd overload, we'd just add new parameters to the function
signature.


>   The
> present design is also fragile in that an unrecognized parameter
> will lead to a parse-time failure and no function call happening,
> which is less robust than I'd like.


There was a lot of back-and-forth about what sorts of failures were
error-worthy, and which were warn-worthy. I'll discuss further below.


>   As lesser points,
> the relation argument ought to be declared regclass not oid for
> convenience of use,


+1


> and I really think that we need to provide
> the source server's major version number --- maybe we will never
> need that, but if we do and we don't have it we will be sad.
>

The JSON had it, and I never did use it. Not against having it again.


>
> So this leads me to suggest that we'd be best off with a VARIADIC
> ANY signature, where the variadic part consists of alternating
> parameter labels and values:
>
> pg_set_attribute_stats(table regclass, attribute name,
>inherited bool, source_version int,
>variadic "any") returns void
>
> where a call might look like
>
> SELECT pg_set_attribute_stats('public.mytable', 'mycolumn',
>   false, -- not inherited
>   16, -- source server major version
>   -- pairs of labels and values follow
>   'null_frac', 0.4,
>   'avg_width', 42,
>   'histogram_bounds',
>   array['a', 'b', 'c']::text[],
>   ...);
>
> Note a couple of useful things here:
>
> * AFAICS we could label the function strict and remove all those ad-hoc
> null checks.  If you don't have a value for a particular stat, you
> just leave that pair of arguments out (exactly as the existing code
> in 0002 does, just using a different notation).  This also means that
> we don't need any default arguments and so no need for hackery in
> system_functions.sql.
>

I'm not aware of how strict works with variadics. Would the lack of any
variadic parameters trigger it?

Also going with strict means that an inadvertent explicit NULL in one
parameter would cause the entire attribute import to fail silently. I'd
rather fail loudly.



> * If we don't recognize a parameter label at runtime, we can treat
> that as a warning rather than a hard error, and press on.  This case
> would mostly be useful in major version downgrades I suppose, but
> that will be something people will want eventually.
>

Interesting.

* We can require the calling statement to cast arguments, particularly
> arrays, to the proper type, removing the need for conversions within
> the stats-setting function.  (But instead, it'd need to check that the
> next "any" argument is the type it ought to be based on the type of
> the target column.)
>

So, that's tricky. The type of the values is not always the attribute type,
for expression indexes, we do call exprType() and exprCollation(), in which
case we always use the expression type over the attribute type, but only
use the collation type if the attribute had no collation. This mimics the
behavior of ANALYZE.

Then, for the MCELEM and DECHIST stakinds we have to find the type's
element type, and that has special logic for tsvectors, ranges, and other
non-scalars, borrowing from the various *_typanalyze() functions. For that
matter, the existing typanalyze functions don't grab the < operator, which
I need for later data validations, so using examine_attribute() was
simultaneously overkill and insufficient.

None of this functionality is accessible from a client program, so we'd
have to pull in a lot of backend stuff to pg_dump to make it resolve the
typecasts correctly. Text and array_in() was just easier.


> pg_set_relation_stats is simpler in that the set of stats values
> to be set will probably remain fairly static, and there seems little
> reason to allow 

Re: Statistics Import and Export

[Corey Huinker]

>
> I wonder if the right answer to that is "let's enhance the I/O
> functions for those types".  But whether that helps or not, it's
> v18-or-later material for sure.
>

That was Stephen's take as well, and I agreed given that I had to throw the
kitchen-sink of source-side oid mappings (attname, types, collatons,
operators) into the JSON to work around the limitation.


> I can't quibble with that view of what has priority.  I'm just
> suggesting that redesigning what pg_upgrade does in this area
> should come later than doing something about extended stats.
>

I mostly agree, with the caveat that pg_upgrade's existing message saying
that optimizer stats were not carried over wouldn't be 100% true anymore.

Re: Statistics Import and Export

[Corey Huinker]

On Sun, Mar 31, 2024 at 2:41 PM Tom Lane  wrote:

> Corey Huinker  writes:
> > Having given this some thought, I'd be inclined to create a view,
> > pg_stats_missing, with the same security barrier as pg_stats, but looking
> > for tables that lack stats on at least one column, or lack stats on an
> > extended statistics object.
>
> The week before feature freeze is no time to be designing something
> like that, unless you've abandoned all hope of getting this into v17.
>

It was a response to the suggestion that there be some way for
tools/automation to read the status of stats. I would view it as a separate
patch, as such a view would be useful now for knowing which tables to
ANALYZE, regardless of whether this patch goes in or not.


> There's a bigger issue though: AFAICS this patch set does nothing
> about dumping extended statistics.  I surely don't want to hold up
> the patch insisting that that has to happen before we can commit the
> functionality proposed here.  But we cannot rip out pg_upgrade's
> support for post-upgrade ANALYZE processing before we do something
> about extended statistics, and that means it's premature to be
> designing any changes to how that works.  So I'd set that whole
> topic on the back burner.
>

So Extended Stats _were_ supported by earlier versions where the medium of
communication was JSON. However, there were several problems with adapting
that to the current model where we match params to stat types:

* Several of the column types do not have functional input functions, so we
must construct the data structure internally and pass them to
statext_store().
* The output functions for some of those column types have lists of
attnums, with negative values representing positional expressions in the
stat definition. This information is not translatable to another system
without also passing along the attnum/attname mapping of the source system.

At least three people told me "nobody uses extended stats" and to just drop
that from the initial version. Unhappy with this assessment, I inquired as
to whether my employer (AWS) had some internal databases that used extended
stats so that I could get good test data, and came up with nothing, nor did
anyone know of customers who used the feature. So when the fourth person
told me that nobody uses extended stats, and not to let a rarely-used
feature get in the way of a feature that would benefit nearly 100% of
users, I dropped it.


> It's possible that we could drop the analyze-in-stages recommendation,
> figuring that this functionality will get people to the
> able-to-limp-along level immediately and that all that is needed is a
> single mop-up ANALYZE pass.  But I think we should leave that till we
> have a bit more than zero field experience with this feature.


It may be that we leave the recommendation exactly as it is.

Perhaps we enhance the error messages in pg_set_*_stats() to indicate what
command would remediate the issue.

Re: Statistics Import and Export

[Corey Huinker]

>
> That will make it *really* hard for any form of automation or drivers of
> this. The information needs to go somewhere where such tools can easily
> consume it, and an informational message during runtime (which is also
> likely to be translated in many environments) is the exact opposite of that.
>

Having given this some thought, I'd be inclined to create a view,
pg_stats_missing, with the same security barrier as pg_stats, but looking
for tables that lack stats on at least one column, or lack stats on an
extended statistics object.

Table structure would be

schemaname name
tablename name
attnames text[]
ext_stats text[]


The informational message, if it changes at all, could reference this new
view as the place to learn about how well the stats import went.

vacuumdb might get a --missing-only option in addition to
--analyze-in-stages.

For that matter, we could add --analyze-missing options to pg_restore and
pg_upgrade to do the mopping up themselves.

Re: Statistics Import and Export

[Corey Huinker]

>
> I didn't have any specific proposal in mind, was just trying to think
> outside the box.
>

What if we added a separate resection SECTION_STATISTICS which is run
following post-data?

Re: Statistics Import and Export

[Corey Huinker]

>
> I'm getting late into this discussion and I apologize if I've missed this
> being discussed before. But.
>
> Please don't.
>
> That will make it *really* hard for any form of automation or drivers of
> this. The information needs to go somewhere where such tools can easily
> consume it, and an informational message during runtime (which is also
> likely to be translated in many environments) is the exact opposite of that.
>

That makes a lot of sense. I'm not sure what form it would take (file,
pseudo-table, something else?). Open to suggestions.

Re: Statistics Import and Export

[Corey Huinker]

>
> (I've not read the patch yet, but I assume the switch works like
> other pg_dump filters in that you can apply it on the restore
> side?)
>

Correct. It follows the existing --no-something pattern.

Re: Statistics Import and Export

[Corey Huinker]

On Fri, Mar 29, 2024 at 7:34 PM Jeff Davis  wrote:

> On Fri, 2024-03-29 at 18:02 -0400, Stephen Frost wrote:
> > I’d certainly think “with stats” would be the preferred default of
> > our users.
>
> I'm concerned there could still be paths that lead to an error. For
> pg_restore, or when loading a SQL file, a single error isn't fatal
> (unless -e is specified), but it still could be somewhat scary to see
> errors during a reload.
>

To that end, I'm going to be modifying the "Optimizer statistics are not
transferred by pg_upgrade..." message when stats _were_ transferred,
width additional instructions that the user should treat any stats-ish
error messages encountered as a reason to manually analyze that table. We
should probably say something about extended stats as well.

Re: Statistics Import and Export

[Corey Huinker]

>
>
> There's still a failure in the pg_upgrade TAP test. One seems to be
> ordering, so perhaps we need to ORDER BY the attribute number. Others
> seem to be missing relstats and I'm not sure why yet. I suggest doing
> some manual pg_upgrade tests and comparing the before/after dumps to
> see if you can reproduce a smaller version of the problem.
>

That's fixed in my current working version, as is a tsvector-specific
issue. Working on the TAP issue.

Re: Statistics Import and Export

[Corey Huinker]

>
> 1) The docs say this:
>
>   
>The purpose of this function is to apply statistics values in an
>upgrade situation that are "good enough" for system operation until
>they are replaced by the next ANALYZE, usually via
>autovacuum This function is used by
>pg_upgrade and pg_restore to
>convey the statistics from the old system version into the new one.
>   
>
> I find this a bit confusing, considering the pg_dump/pg_restore changes
> are only in 0002, not in this patch.
>

True, I'll split the docs.


>
> 2) Also, I'm not sure about this:
>
>   relation, the parameters in this are all
>   derived from pg_stats, and the values
>   given are most often extracted from there.
>
> How do we know where do the values come from "most often"? I mean, where
> else would it come from?
>

The next most likely sources would be 1. stats from another similar table
and 2. the imagination of a user testing hypothetical query plans.


>
> 3) The function pg_set_attribute_stats() is very long - 1000 lines
> or so, that's way too many for me to think about. I agree the flow is
> pretty simple, but I still wonder if there's a way to maybe split it
> into some smaller "meaningful" steps.
>

I wrestle with that myself. I think there's some pieces that can be
factored out.


> 4) It took me *ages* to realize the enums at the beginning of some of
> the functions are actually indexes of arguments in PG_FUNCTION_ARGS.
> That'd surely deserve a comment explaining this.
>

My apologies, it definitely deserves a comment.


>
> 5) The comment for param_names in pg_set_attribute_stats says this:
>
> /* names of columns that cannot be null */
> const char *param_names[] = { ... }
>
> but isn't that actually incorrect? I think that applies only to a couple
> initial arguments, but then other fields (MCV, mcelem stats, ...) can be
> NULL, right?
>

Yes, that is vestigial, I'll remove it.


>
> 6) There's a couple minor whitespace fixes or comments etc.
>
>
> 0002
> 
>
> 1) I don't understand why we have exportExtStatsSupported(). Seems
> pointless - nothing calls it, even if it did we don't know how to export
> the stats.
>

It's not strictly necessary.


>
> 2) I think this condition in dumpTableSchema() is actually incorrect:
>
> IMO that's wrong, the statistics should be delayed to the post-data
> section. Which probably means there needs to be a separate dumpable
> object for statistics on table/index, with a dependency on the object.
>

Good points.


>
> 3) I don't like the "STATS IMPORT" description. For extended statistics
> we dump the definition as "STATISTICS" so why to shorten it to "STATS"
> here? And "IMPORT" seems more like the process of loading data, not the
> data itself. So I suggest "STATISTICS DATA".
>

+1

Re: Statistics Import and Export

[Corey Huinker]

>
>
>
> +\gexec
>
> Why do we need to construct the command and execute? Can we instead
> execute the function directly? That would also avoid ECHO magic.
>

We don't strictly need it, but I've found the set-difference operation to
be incredibly useful in diagnosing problems. Additionally, the values are
subject to change due to changes in test data, no guarantee that the output
of ANALYZE is deterministic, etc. But most of all, because the test cares
about the correct copying of values, not the values themselves.


>
> +   
> +Database Object Statistics Import Functions
> +
> + 
> +  
> +   
> +Function
> +   
> +   
> +Description
> +   
> +  
> + 
>
> COMMENT: The functions throw many validation errors. Do we want to list
> the acceptable/unacceptable input values in the documentation corresponding
> to those? I don't expect one line per argument validation. Something like
> "these, these and these arguments can not be NULL" or "both arguments in
> each of the pairs x and y, a and b, and c and d should be non-NULL or NULL
> respectively".
>

Yes. It should.


>  Statistics are about data. Whenever pg_dump dumps some filtered data, the
> statistics collected for the whole table are uselss. We should avoide
> dumping
> statistics in such a case. E.g. when only schema is dumped what good is
> statistics? Similarly the statistics on a partitioned table may not be
> useful
> if some its partitions are not dumped. Said that dumping statistics on
> foreign
> table makes sense since they do not contain data but the statistics still
> makes sense.
>

Good points, but I'm not immediately sure how to enforce those rules.


>
>
>>
>> Key areas where I'm seeking feedback:
>>
>> - What level of errors in a restore will a user tolerate, and what should
>> be done to the error messages to indicate that the data itself is fine, but
>> a manual operation to update stats on that particular table is now
>> warranted?
>> - To what degree could pg_restore/pg_upgrade take that recovery action
>> automatically?
>> - Should the individual attribute/class set function calls be grouped by
>> relation, so that they all succeed/fail together, or should they be called
>> separately, each able to succeed or fail on their own?
>> - Any other concerns about how to best use these new functions.
>>
>>
>>
> Whether or not I pass --no-statistics, there is no difference in the dump
> output. Am I missing something?
> $ pg_dump -d postgres > /tmp/dump_no_arguments.out
> $ pg_dump -d postgres --no-statistics > /tmp/dump_no_statistics.out
> $ diff /tmp/dump_no_arguments.out /tmp/dump_no_statistics.out
> $
>
> IIUC, pg_dump includes statistics by default. That means all our pg_dump
> related tests will have statistics output by default. That's good since the
> functionality will always be tested. 1. We need additional tests to ensure
> that the statistics is installed after restore. 2. Some of those tests
> compare dumps before and after restore. In case the statistics is changed
> because of auto-analyze happening post-restore, these tests will fail.
>

+1


> I believe, in order to import statistics through IMPORT FOREIGN SCHEMA,
> postgresImportForeignSchema() will need to add SELECT commands invoking
> pg_set_relation_stats() on each imported table and pg_set_attribute_stats()
> on each of its attribute. Am I right? Do we want to make that happen in the
> first cut of the feature? How do you expect these functions to be used to
> update statistics of foreign tables?
>

I don't think there's time to get it into this release. I think we'd want
to extend this functionality to both IMPORT FOREIGN SCHEMA and ANALYZE for
foreign tables, in both cases with a server/table option to do regular
remote sampling. In both cases, they'd do a remote query very similar to
what pg_dump does (hence putting it in fe_utils), with some filters on
which columns/tables it believes it can trust. The remote table might
itself be a view (in which case they query would turn up nothing) or column
data types may change across the wire, and in those cases we'd have to fall
back to sampling.

Re: Statistics Import and Export

[Corey Huinker]

>
>
>
> But ideally we'd just make it safe to dump and reload stats on your own
> tables, and then not worry about it.
>

That is my strong preference, yes.


>
> > Not off hand, no.
>
> To me it seems like inconsistent data to have most_common_freqs in
> anything but descending order, and we should prevent it.
>

Sorry, I misunderstood, I thought we were talking about values, not the
frequencies. Yes, the frequencies should only be monotonically
non-increasing (i.e. it can go down or flatline from N->N+1). I'll add a
test case for that.

Re: Statistics Import and Export

[Corey Huinker]

>
> How about just some defaults then? Many of them have a reasonable
> default, like NULL or an empty array. Some are parallel arrays and
> either both should be specified or neither (e.g.
> most_common_vals+most_common_freqs), but you can check for that.
>

+1
Default NULL has been implemented for all parameters after n_distinct.


>
> > > Why are you calling checkCanModifyRelation() twice?
> >
> > Once for the relation itself, and once for pg_statistic.
>
> Nobody has the privileges to modify pg_statistic except superuser,
> right? I thought the point of a privilege check is that users could
> modify statistics for their own tables, or the tables they maintain.
>

In which case wouldn't the checkCanModify on pg_statistic would be a proxy
for is_superuser/has_special_role_we_havent_created_yet.



>
> >
> > I can see making it void and returning an error for everything that
> > we currently return false for, but if we do that, then a statement
> > with one pg_set_relation_stats, and N pg_set_attribute_stats (which
> > we lump together in one command for the locking benefits and atomic
> > transaction) would fail entirely if one of the set_attributes named a
> > column that we had dropped. It's up for debate whether that's the
> > right behavior or not.
>
> I'd probably make the dropped column a WARNING with a message like
> "skipping dropped column whatever". Regardless, have some kind of
> explanatory comment.
>

That's certainly do-able.




>
> >
> > I pulled most of the hardcoded values from pg_stats itself. The
> > sample set is trivially small, and the values inserted were in-order-
> > ish. So maybe that's why.
>
> In my simple test, most_common_freqs is descending:
>
>CREATE TABLE a(i int);
>INSERT INTO a VALUES(1);
>INSERT INTO a VALUES(2);
>INSERT INTO a VALUES(2);
>INSERT INTO a VALUES(3);
>INSERT INTO a VALUES(3);
>INSERT INTO a VALUES(3);
>INSERT INTO a VALUES(4);
>INSERT INTO a VALUES(4);
>INSERT INTO a VALUES(4);
>INSERT INTO a VALUES(4);
>ANALYZE a;
>SELECT most_common_vals, most_common_freqs
>  FROM pg_stats WHERE tablename='a';
> most_common_vals | most_common_freqs
>--+---
> {4,3,2}  | {0.4,0.3,0.2}
>(1 row)
>
> Can you show an example where it's not?
>

Not off hand, no.



>
> >
> > Maybe we could have the functions restricted to a role or roles:
> >
> > 1. pg_write_all_stats (can modify stats on ANY table)
> > 2. pg_write_own_stats (can modify stats on tables owned by user)
>
> If we go that route, we are giving up on the ability for users to
> restore stats on their own tables. Let's just be careful about
> validating data to mitigate this risk.
>

A great many test cases coming in the next patch.

Re: Statistics Import and Export

[Corey Huinker]

On Thu, Mar 21, 2024 at 2:29 AM Jeff Davis  wrote:

> On Tue, 2024-03-19 at 05:16 -0400, Corey Huinker wrote:
> > v11 attached.
>
> Thank you.
>
> Comments on 0001:
>
> This test:
>
>+SELECT
>+format('SELECT pg_catalog.pg_set_attribute_stats( '
>...
>
> seems misplaced. It's generating SQL that can be used to restore or
> copy the stats -- that seems like the job of pg_dump, and shouldn't be
> tested within the plain SQL regression tests.
>

Fair enough.


>
> And can the other tests use pg_stats rather than pg_statistic?
>

They can, but part of what I wanted to show was that the values that aren't
directly passed in as parameters (staopN, stacollN) get set to the correct
values, and those values aren't guaranteed to match across databases, hence
testing them in the regression test rather than in a TAP test. I'd still
like to be able to test that.


>
> The function signature for pg_set_attribute_stats could be more
> friendly -- how about there are a few required parameters, and then it
> only sets the stats that are provided and the other ones are either
> left to the existing value or get some reasonable default?
>

That would be problematic.

1. We'd have to compare the stats provided against the stats that are
already there, make that list in-memory, and then re-order what remains
2. There would be no way to un-set statistics of a given stakind, unless we
added an "actually set it null" boolean for each parameter that can be
null.
3. I tried that with the JSON formats, it made the code even messier than
it already was.

Make sure all error paths ReleaseSysCache().
>

+1


>
> Why are you calling checkCanModifyRelation() twice?
>

Once for the relation itself, and once for pg_statistic.


> I'm confused about when the function should return false and when it
> should throw an error. I'm inclined to think the return type should be
> void and all failures should be reported as ERROR.
>

I go back and forth on that. I can see making it void and returning an
error for everything that we currently return false for, but if we do that,
then a statement with one pg_set_relation_stats, and N
pg_set_attribute_stats (which we lump together in one command for the
locking benefits and atomic transaction) would fail entirely if one of the
set_attributes named a column that we had dropped. It's up for debate
whether that's the right behavior or not.

replaces[] is initialized to {true}, which means only the first element
> is initialized to true. Try following the pattern in AlterDatabase (or
> similar) which reads the catalog tuple first, then updates a few fields
> selectively, setting the corresponding element of replaces[] along the
> way.
>

+1.


>
> The test also sets the most_common_freqs in an ascending order, which
> is weird.
>

I pulled most of the hardcoded values from pg_stats itself. The sample set
is trivially small, and the values inserted were in-order-ish. So maybe
that's why.


> Relatedly, I got worried recently about the idea of plain users
> updating statistics. In theory, that should be fine, and the planner
> should be robust to whatever pg_statistic contains; but in practice
> there's some risk of mischief there until everyone understands that the
> contents of pg_stats should not be trusted. Fortunately I didn't find
> any planner crashes or even errors after a brief test.
>

Maybe we could have the functions restricted to a role or roles:

1. pg_write_all_stats (can modify stats on ANY table)
2. pg_write_own_stats (can modify stats on tables owned by user)

I'm iffy on the need for the first one, I list it first purely to show how
I derived the name for the second.


> One thing we can do is some extra validation for consistency, like
> checking that the arrays are properly sorted, check for negative
> numbers in the wrong place, or fractions larger than 1.0, etc.
>

+1. All suggestions of validation checks welcome.

Re: Statistics Import and Export

[Corey Huinker]

v11 attached.

- TAP tests passing (the big glitch was that indexes that are used in
constraints should have their stats dependent on the constraint, not the
index, thanks Jeff)
- The new range-specific statistics types are now supported. I'm not happy
with the typid machinations I do to get them to work, but it is working so
far. These are stored out-of-stakind-order (7 before 6), which is odd
because all other types seem store stakinds in ascending order. It
shouldn't matter, it was just odd.
- regression tests now make simpler calls with arbitrary stats to
demonstrate the function usage more cleanly
- pg_set_*_stats function now have all of their parameters in the same
order as the table/view they pull from
From 5c63ed5748eb3817d193b64329b57dc590e0196e Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Mon, 11 Mar 2024 14:18:39 -0400
Subject: [PATCH v11 1/2] Create pg_set_relation_stats, pg_set_attribute_stats.

These functions will be used by pg_dump/restore and pg_upgrade to convey
relation and attribute statistics from the source database to the
target. This would be done instead of vacuumdb --analyze-in-stages.

Both functions take an oid to identify the target relation that will
receive the statistics. There is nothing requiring that relation to be
the same one as the one exported, though the statistics would have to
make sense in the context of the new relation.

The parameters of pg_set_attribute_stats intentionaly mirror the columns
in the view pg_stats, with the ANYARRAY types casted to TEXT. Those
values will be cast to arrays of the basetype of the attribute, and that
operation may fail if the attribute type has changed.

The statistics imported by pg_set_attribute_stats are imported
transactionally like any other operation. However, pg_set_relation_stats
does it's update in-place, which is to say non-transactionally. This is
in line with what ANALYZE does to avoid table bloat in pg_class.

These functions also allows for tweaking of table statistics in-place,
allowing the user to inflate rowcounts, skew histograms, etc, to see
what those changes will evoke from the query planner.
---
 src/include/catalog/pg_proc.dat   |  15 +
 src/include/statistics/statistics.h   |   2 +
 src/backend/statistics/Makefile   |   3 +-
 src/backend/statistics/meson.build|   1 +
 src/backend/statistics/statistics.c   | 603 ++
 .../regress/expected/stats_export_import.out  | 283 
 src/test/regress/parallel_schedule|   2 +-
 src/test/regress/sql/stats_export_import.sql  | 246 +++
 doc/src/sgml/func.sgml|  91 +++
 9 files changed, 1244 insertions(+), 2 deletions(-)
 create mode 100644 src/backend/statistics/statistics.c
 create mode 100644 src/test/regress/expected/stats_export_import.out
 create mode 100644 src/test/regress/sql/stats_export_import.sql

diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 177d81a891..f31412d4a6 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -12177,4 +12177,19 @@
   proargtypes => 'int2',
   prosrc => 'gist_stratnum_identity' },
 
+# Statistics Import
+{ oid => '8048',
+  descr => 'set statistics on relation',
+  proname => 'pg_set_relation_stats', provolatile => 'v', proisstrict => 't',
+  proparallel => 'u', prorettype => 'bool',
+  proargtypes => 'oid int4 float4 int4',
+  proargnames => '{relation,relpages,reltuples,relallvisible}',
+  prosrc => 'pg_set_relation_stats' },
+{ oid => '8049',
+  descr => 'set statistics on attribute',
+  proname => 'pg_set_attribute_stats', provolatile => 'v', proisstrict => 'f',
+  proparallel => 'u', prorettype => 'bool',
+  proargtypes => 'oid name bool float4 int4 float4 text _float4 text float4 text _float4 _float4 text float4 text',
+  proargnames => '{relation,attname,inherited,null_frac,avg_width,n_distinct,most_common_vals,most_common_freqs,histogram_bounds,correlation,most_common_elems,most_common_elem_freqs,elem_count_histogram,range_length_histogram,range_empty_frac,range_bounds_histogram}',
+  prosrc => 'pg_set_attribute_stats' },
 ]
diff --git a/src/include/statistics/statistics.h b/src/include/statistics/statistics.h
index 7f2bf18716..1dddf96576 100644
--- a/src/include/statistics/statistics.h
+++ b/src/include/statistics/statistics.h
@@ -127,4 +127,6 @@ extern StatisticExtInfo *choose_best_statistics(List *stats, char requiredkind,
 int nclauses);
 extern HeapTuple statext_expressions_load(Oid stxoid, bool inh, int idx);
 
+extern Datum pg_set_relation_stats(PG_FUNCTION_ARGS);
+extern Datum pg_set_attribute_stats(PG_FUNCTION_ARGS);
 #endif			/* STATISTICS_H */
diff --git a/src/backend/statistics/Makefile b/src/backend/statistics/Makefile
index 89cf8c2797..e4f8ab7c4f 100644
--- a/src/backend/statistics/Makefile
+++ b/src/backend/statistics/Makefile
@@ -16,6

Re: Statistics Import and Export

[Corey Huinker]

>
>
>
>
> From testrun/pg_dump/002_pg_dump/log/regress_log_002_pg_dump, search
> for the "not ok" and then look at what it tried to do right before
> that. I see:
>
> pg_dump: error: prepared statement failed: ERROR:  syntax error at or
> near "%"
> LINE 1: ..._histogram => %L::real[]) coalesce($2, format('%I.%I',
> a.nsp...
>

Thanks. Unfamiliar turf for me.


>
> > All those changes are available in the patches attached.
>
> How about if you provided "get" versions of the functions that return a
> set of rows that match what the "set" versions expect? That would make
> 0001 essentially a complete feature itself.
>

That's tricky. At the base level, those functions would just be an
encapsulation of "SELECT * FROM pg_stats WHERE schemaname = $1 AND
tablename = $2" which isn't all that much of a savings. Perhaps we can make
the documentation more explicit about the source and nature of the
parameters going into the pg_set_ functions.

Per conversation, it would be trivial to add a helper functions that
replace the parameters after the initial oid with a pg_class rowtype, and
that would dissect the values needed and call the more complex function:

pg_set_relation_stats( oid, pg_class)
pg_set_attribute_stats( oid, pg_stats)


>
> I think it would also make the changes in pg_dump simpler, and the
> tests in 0001 a lot simpler.
>

I agree. The tests are currently showing that a fidelity copy can be made
from one table to another, but to do so we have to conceal the actual stats
values because those are 1. not deterministic/known and 2. subject to
change from version to version.

I can add some sets to arbitrary values like was done for
pg_set_relation_stats().

Re: Statistics Import and Export

[Corey Huinker]

>
>
> * pg_set_relation_stats() needs to do an ACL check so you can't set the
> stats on someone else's table. I suggest honoring the new MAINTAIN
> privilege as well.
>

Added.


>
> * If possible, reading from pg_stats (instead of pg_statistic) would be
> ideal because pg_stats already does the right checks at read time, so a
> non-superuser can export stats, too.
>

Done. That was sorta how it was originally, so returning to that wasn't too
hard.


>
> * If reading from pg_stats, should you change the signature of
> pg_set_relation_stats() to have argument names matching the columns of
> pg_stats (e.g. most_common_vals instead of stakind/stavalues)?
>

Done.


>
> In other words, make this a slightly higher level: conceptually
> exporting/importing pg_stats rather than pg_statistic. This may also
> make the SQL export queries simpler.
>

Eh, about the same.


> Also, I'm wondering about error handling. Is some kind of error thrown
> by pg_set_relation_stats() going to abort an entire restore? That might
> be easy to prevent with pg_restore, because it can just omit the stats,
> but harder if it's in a SQL file.
>

Aside from the oid being invalid, there's not a whole lot that can go wrong
in set_relation_stats(). The error checking I did closely mirrors that in
analyze.c.

Aside from the changes you suggested, as well as the error reporting change
you suggested for pg_dump, I also filtered out attempts to dump stats on
views.

A few TAP tests are still failing and I haven't been able to diagnose why,
though the failures in parallel dump seem to be that it tries to import
stats on indexes that haven't been created yet, which is odd because I sent
the dependency.

All those changes are available in the patches attached.
From ba411ce31c25193cf05bc4206dcb8f2bf32af0c7 Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Mon, 11 Mar 2024 14:18:39 -0400
Subject: [PATCH v10 1/2] Create pg_set_relation_stats, pg_set_attribute_stats.

These functions will be used by pg_dump/restore and pg_upgrade to convey
relation and attribute statistics from the source database to the
target. This would be done instead of vacuumdb --analyze-in-stages.

Both functions take an oid to identify the target relation that will
receive the statistics. There is nothing requiring that relation to be
the same one as the one exported, though the statistics would have to
make sense in the context of the new relation.

The parameters of pg_set_attribute_stats intentionaly mirror the columns
in the view pg_stats, with the ANYARRAY types casted to TEXT. Those
values will be cast to arrays of the basetype of the attribute, and that
operation may fail if the attribute type has changed.

The statistics imported by pg_set_attribute_stats are imported
transactionally like any other operation. However, pg_set_relation_stats
does it's update in-place, which is to say non-transactionally. This is
in line with what ANALYZE does to avoid table bloat in pg_class.

These functions also allows for tweaking of table statistics in-place,
allowing the user to inflate rowcounts, skew histograms, etc, to see
what those changes will evoke from the query planner.
---
 src/include/catalog/pg_proc.dat   |  15 +
 src/include/statistics/statistics.h   |   2 +
 src/backend/statistics/Makefile   |   3 +-
 src/backend/statistics/meson.build|   1 +
 src/backend/statistics/statistics.c   | 521 ++
 .../regress/expected/stats_export_import.out  | 211 +++
 src/test/regress/parallel_schedule|   2 +-
 src/test/regress/sql/stats_export_import.sql  | 188 +++
 doc/src/sgml/func.sgml|  83 +++
 9 files changed, 1024 insertions(+), 2 deletions(-)
 create mode 100644 src/backend/statistics/statistics.c
 create mode 100644 src/test/regress/expected/stats_export_import.out
 create mode 100644 src/test/regress/sql/stats_export_import.sql

diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 700f7daf7b..3070d72d7a 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -12170,4 +12170,19 @@
   proargtypes => 'int2',
   prosrc => 'gist_stratnum_identity' },
 
+# Statistics Import
+{ oid => '8048',
+  descr => 'set statistics on relation',
+  proname => 'pg_set_relation_stats', provolatile => 'v', proisstrict => 't',
+  proparallel => 'u', prorettype => 'bool',
+  proargtypes => 'oid float4 int4 int4',
+  proargnames => '{relation,reltuples,relpages,relallvisible}',
+  prosrc => 'pg_set_relation_stats' },
+{ oid => '8049',
+  descr => 'set statistics on attribute',
+  proname => 'pg_set_attribute_stats', provolatile => 'v', proisstrict => 'f',
+  proparallel => 'u', prorettype => 'bool',
+  proargtypes => 'oid name bool float4 int4 float4 text _float4 text float4 text _float4 _

Re: Statistics Import and Export

[Corey Huinker]

>
> ANALYZE takes out one lock StatisticRelationId per relation, not per
> attribute like we do now. If we didn't release the lock after every
> attribute, and we only called the function outside of a larger transaction
> (as we plan to do with pg_restore) then that is the closest we're going to
> get to being consistent with ANALYZE.
>

v9 attached. This adds pg_dump support. It works in tests against existing
databases such as dvdrental, though I was surprised at how few indexes have
attribute stats there.

Statistics are preserved by default, but this can be disabled with the
option --no-statistics. This follows the prevailing option pattern in
pg_dump, etc.

There are currently several failing TAP tests around
pg_dump/pg_restore/pg_upgrade. I'm looking at those, but in the mean
time I'm seeking feedback on the progress so far.
From bf291e323fc01215264d41b75d579c11bd22d2ec Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Mon, 11 Mar 2024 14:18:39 -0400
Subject: [PATCH v9 1/2] Create pg_set_relation_stats, pg_set_attribute_stats.

These functions will be used by pg_dump/restore and pg_upgrade to convey
relation and attribute statistics from the source database to the
target. This would be done instead of vacuumdb --analyze-in-stages.

Both functions take an oid to identify the target relation that will
receive the statistics. There is nothing requiring that relation to be
the same one as the one exported, though the statistics would have to
make sense in the context of the new relation. Typecasts for stavaluesN
parameters may fail if the destination column is not of the same type as
the source column.

The parameters of pg_set_attribute_stats identify the attribute by name
rather than by attnum. This is intentional because the column order may
be different in situations other than binary upgrades. For example,
dropped columns do not carry over in a restore.

The statistics imported by pg_set_attribute_stats are imported
transactionally like any other operation. However, pg_set_relation_stats
does it's update in-place, which is to say non-transactionally. This is
in line with what ANALYZE does to avoid table bloat in pg_class.

These functions also allows for tweaking of table statistics in-place,
allowing the user to inflate rowcounts, skew histograms, etc, to see
what those changes will evoke from the query planner.
---
 src/include/catalog/pg_proc.dat   |  15 +
 src/include/statistics/statistics.h   |   2 +
 src/backend/statistics/Makefile   |   3 +-
 src/backend/statistics/meson.build|   1 +
 src/backend/statistics/statistics.c   | 410 ++
 .../regress/expected/stats_export_import.out  | 211 +
 src/test/regress/parallel_schedule|   2 +-
 src/test/regress/sql/stats_export_import.sql  | 198 +
 doc/src/sgml/func.sgml|  95 
 9 files changed, 935 insertions(+), 2 deletions(-)
 create mode 100644 src/backend/statistics/statistics.c
 create mode 100644 src/test/regress/expected/stats_export_import.out
 create mode 100644 src/test/regress/sql/stats_export_import.sql

diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 700f7daf7b..a726451a6f 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -12170,4 +12170,19 @@
   proargtypes => 'int2',
   prosrc => 'gist_stratnum_identity' },
 
+# Statistics Import
+{ oid => '8048',
+  descr => 'set statistics on relation',
+  proname => 'pg_set_relation_stats', provolatile => 'v', proisstrict => 't',
+  proparallel => 'u', prorettype => 'bool',
+  proargtypes => 'oid float4 int4 int4',
+  proargnames => '{relation,reltuples,relpages,relallvisible}',
+  prosrc => 'pg_set_relation_stats' },
+{ oid => '8049',
+  descr => 'set statistics on attribute',
+  proname => 'pg_set_attribute_stats', provolatile => 'v', proisstrict => 'f',
+  proparallel => 'u', prorettype => 'bool',
+  proargtypes => 'oid name bool float4 int4 float4 int2 int2 int2 int2 int2 _float4 _float4 _float4 _float4 _float4 text text text text text',
+  proargnames => '{relation,attname,stainherit,stanullfrac,stawidth,stadistinct,stakind1,stakind2,stakind3,stakind4,stakind5,stanumbers1,stanumbers2,stanumbers3,stanumbers4,stanumbers5,stavalues1,stavalues2,stavalues3,stavalues4,stavalues5}',
+  prosrc => 'pg_set_attribute_stats' },
 ]
diff --git a/src/include/statistics/statistics.h b/src/include/statistics/statistics.h
index 7f2bf18716..1dddf96576 100644
--- a/src/include/statistics/statistics.h
+++ b/src/include/statistics/statistics.h
@@ -127,4 +127,6 @@ extern StatisticExtInfo *choose_best_statistics(List *stats, char requiredkind,
 int nclauses);
 extern HeapTuple statext_expressions_load(Oid stxoid, bool inh, int idx);
 
+extern Datum pg_set_relation_stats(PG_FUNCTION_ARGS);
+extern Datum pg_set_attribute_stats(

Re: Statistics Import and Export

[Corey Huinker]

>
> Note that there's two different things we're talking about here- the
> lock on the relation that we're analyzing and then the lock on the
> pg_statistic (or pg_class) catalog itself.  Currently, at least, it
> looks like in the three places in the backend that we open
> StatisticRelationId, we release the lock when we close it rather than
> waiting for transaction end.  I'd be inclined to keep it that way in
> these functions also.  I doubt that one lock will end up causing much in
> the way of issues to acquire/release it multiple times and it would keep
> the code consistent with the way ANALYZE works.
>

ANALYZE takes out one lock StatisticRelationId per relation, not per
attribute like we do now. If we didn't release the lock after every
attribute, and we only called the function outside of a larger transaction
(as we plan to do with pg_restore) then that is the closest we're going to
get to being consistent with ANALYZE.

Re: Statistics Import and Export

[Corey Huinker]

>
> No, we should be keeping the lock until the end of the transaction
> (which in this case would be just the one statement, but it would be the
> whole statement and all of the calls in it).  See analyze.c:268 or
> so, where we call relation_close(onerel, NoLock); meaning we're closing
> the relation but we're *not* releasing the lock on it- it'll get
> released at the end of the transaction.
>
>
If that's the case, then changing the two table_close() statements to
NoLock should resolve any remaining concern.

Re: Statistics Import and Export

[Corey Huinker]

>
> > In which case we're failing nearly silently, yes, there is a null
> returned,
> > but we have no idea why there is a null returned. If I were using this
> > function manually I'd want to know what I did wrong, what parameter I
> > skipped, etc.
>
> I can see it both ways and don't feel super strongly about it ... I just
> know that I've had some cases where we returned an ERROR or otherwise
> were a bit noisy on NULL values getting passed into a function and it
> was much more on the annoying side than on the helpful side; to the
> point where we've gone back and pulled out ereport(ERROR) calls from
> functions before because they were causing issues in otherwise pretty
> reasonable queries (consider things like functions getting pushed down
> to below WHERE clauses and such...).
>

I don't have strong feelings either. I think we should get more input on
this. Regardless, it's easy to change...for now.



>
> Sure.  Not a huge deal either way, was just pointing out the difference.
> I do think it'd be good to match what ANALYZE does here, so checking if
> the values in pg_class are different and only updating if they are,
> while keeping the code for pg_statistic where it'll just always update.
>

I agree that mirroring ANALYZE wherever possible is the ideal.



> > I like the symmetry of a consistent interface, but we've already got an
> > asymmetry in that the pg_class update is done non-transactionally (like
> > ANALYZE does).
>
> Don't know that I really consider that to be the same kind of thing when
> it comes to talking about the interface as the other aspects we're
> discussing ...
>

Fair.




>
> > One persistent problem is that there is no _safe equivalent to ARRAY_IN,
> so
> > that can always fail on us, though it should only do so if the string
> > passed in wasn't a valid array input format, or the values in the array
> > can't coerce to the attribute's basetype.
>
> That would happen before we even get to being called and there's not
> much to do about it anyway.
>

Not sure I follow you here. the ARRAY_IN function calls happen once for
every non-null stavaluesN parameter, and it's done inside the function
because the result type could be the base type for a domain/array type, or
could be the type itself. I suppose we could move that determination to the
caller, but then we'd need to call get_base_element_type() inside a client,
and that seems wrong if it's even possible.


> > I should also point out that we've lost the ability to check if the
> export
> > values were of a type, and if the destination column is also of that
> type.
> > That's a non-issue in binary upgrades, but of course if a field changed
> > from integers to text the histograms would now be highly misleading.
> > Thoughts on adding a typname parameter that the function uses as a cheap
> > validity check?
>
> Seems reasonable to me.
>

I'd like to hear what Tomas thinks about this, as he was the initial
advocate for it.


> > As for pg_dump, I'm currently leading toward the TOC entry having either
> a
> > series of commands:
> >
> > SELECT pg_set_relation_stats('foo.bar'::regclass, ...);
> > pg_set_attribute_stats('foo.bar'::regclass, 'id'::name, ...); ...
>
> I'm guessing the above was intended to be SELECT ..; SELECT ..;
>

Yes.


>
> > Or one compound command
> >
> > SELECT pg_set_relation_stats(t.oid, ...)
> >  pg_set_attribute_stats(t.oid, 'id'::name, ...),
> >  pg_set_attribute_stats(t.oid, 'last_name'::name, ...),
> >  ...
> > FROM (VALUES('foo.bar'::regclass)) AS t(oid);
> >
> > The second one has the feature that if any one attribute fails, then the
> > whole update fails, except, of course, for the in-place update of
> pg_class.
> > This avoids having an explicit transaction block, but we could get that
> > back by having restore wrap the list of commands in a transaction block
> > (and adding the explicit lock commands) when it is safe to do so.
>
> Hm, I like this approach as it should essentially give us the
> transaction block we had been talking about wanting but without needing
> to explicitly do a begin/commit, which would add in some annoying
> complications.  This would hopefully also reduce the locking concern
> mentioned previously, since we'd get the lock needed in the first
> function call and then the others would be able to just see that we've
> already got the lock pretty quickly.
>

True, we'd get the lock needed in the first function call, but wouldn't we
also release that very lock before the subsequent call? Obviously we'd be
shrinking the window in which another process could get in line and take a
superior lock, and the universe of other processes that would even want a
lock that blocks us is nil in the case of an upgrade, identical to existing
behavior in the case of an FDW ANALYZE, and perfectly fine in the case of
someone tinkering with stats.


>
> > Subject: [PATCH v8] Create pg_set_relation_stats, pg_set_attribute_stats.
>
> [...]
>
> > +Datum

Re: Statistics Import and Export

[Corey Huinker]

>
>
>
>
> Having thought about it a bit more, I generally like the idea of being
> able to just update one stat instead of having to update all of them at
> once (and therefore having to go look up what the other values currently
> are...).  That said, per below, perhaps making it strict is the better
> plan.
>

v8 has it as strict.


>
> > > Also, in some cases we allow the function to be called with a
> > > NULL but then make it a no-op rather than throwing an ERROR (eg, if the
> > > OID ends up being NULL).
> >
> > Thoughts on it emitting a WARN or NOTICE before returning false?
>
> Eh, I don't think so?
>
> Where this is coming from is that we can often end up with functions
> like these being called inside of larger queries, and having them spit
> out WARN or NOTICE will just make them noisy.
>
> That leads to my general feeling of just returning NULL if called with a
> NULL OID, as we would get with setting the function strict.
>

In which case we're failing nearly silently, yes, there is a null returned,
but we have no idea why there is a null returned. If I were using this
function manually I'd want to know what I did wrong, what parameter I
skipped, etc.


> Well, that code is for pg_statistic while I was looking at pg_class (in
> vacuum.c:1428-1443, where we track if we're actually changing anything
> and only make the pg_class change if there's actually something
> different):
>

I can do that, especially since it's only 3 tuples of known types, but my
reservations are summed up in the next comment.



> Not sure why we don't treat both the same way though ... although it's
> probably the case that it's much less likely to have an entire
> pg_statistic row be identical than the few values in pg_class.
>

That would also involve comparing ANYARRAY values, yuk. Also, a matched
record will never be the case when used in primary purpose of the function
(upgrades), and not a big deal in the other future cases (if we use it in
ANALYZE on foreign tables instead of remote table samples, users
experimenting with tuning queries under hypothetical workloads).




> Hmm, that's a valid point, so a NULL passed in would need to set that
> value actually to NULL, presumably.  Perhaps then we should have
> pg_set_relation_stats() be strict and have pg_set_attribute_stats()
> handles NULLs passed in appropriately, and return NULL if the relation
> itself or attname, or other required (not NULL'able) argument passed in
> cause the function to return NULL.
>

That's how I have relstats done in v8, and could make it do that for attr
stats.

(What I'm trying to drive at here is a consistent interface for these
> functions, but one which does a no-op instead of returning an ERROR on
> values being passed in which aren't allowable; it can be quite
> frustrating trying to get a query to work where one of the functions
> decides to return ERROR instead of just ignoring things passed in which
> aren't valid.)
>

I like the symmetry of a consistent interface, but we've already got an
asymmetry in that the pg_class update is done non-transactionally (like
ANALYZE does).

One persistent problem is that there is no _safe equivalent to ARRAY_IN, so
that can always fail on us, though it should only do so if the string
passed in wasn't a valid array input format, or the values in the array
can't coerce to the attribute's basetype.

I should also point out that we've lost the ability to check if the export
values were of a type, and if the destination column is also of that type.
That's a non-issue in binary upgrades, but of course if a field changed
from integers to text the histograms would now be highly misleading.
Thoughts on adding a typname parameter that the function uses as a cheap
validity check?

v8 attached, incorporating these suggestions plus those of Bharath and
Bertrand. Still no pg_dump.

As for pg_dump, I'm currently leading toward the TOC entry having either a
series of commands:

SELECT pg_set_relation_stats('foo.bar'::regclass, ...);
pg_set_attribute_stats('foo.bar'::regclass, 'id'::name, ...); ...

Or one compound command

SELECT pg_set_relation_stats(t.oid, ...)
 pg_set_attribute_stats(t.oid, 'id'::name, ...),
 pg_set_attribute_stats(t.oid, 'last_name'::name, ...),
 ...
FROM (VALUES('foo.bar'::regclass)) AS t(oid);

The second one has the feature that if any one attribute fails, then the
whole update fails, except, of course, for the in-place update of pg_class.
This avoids having an explicit transaction block, but we could get that
back by having restore wrap the list of commands in a transaction block
(and adding the explicit lock commands) when it is safe to do so.
From bdfde573f4f79770439a1455c1cb337701eb20dc Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Mon, 11 Mar 2024 14:18:39 -0400
Su

Re: Statistics Import and Export

[Corey Huinker]

On Sun, Mar 10, 2024 at 11:57 AM Bharath Rupireddy <
bharath.rupireddyforpostg...@gmail.com> wrote:

> On Fri, Mar 8, 2024 at 12:06 PM Corey Huinker 
> wrote:
> >
> > Anyway, here's v7. Eagerly awaiting feedback.
>
> Thanks for working on this. It looks useful to have the ability to
> restore the stats after upgrade and restore. But, the exported stats
> are valid only until the next ANALYZE is run on the table. IIUC,
> postgres collects stats during VACUUM, autovacuum and ANALYZE, right?
>  Perhaps there are other ways to collect stats. I'm thinking what
> problems does the user face if they are just asked to run ANALYZE on
> the tables (I'm assuming ANALYZE doesn't block concurrent access to
> the tables) instead of automatically exporting stats.
>

Correct. These are just as temporary as any other analyze of the table.
Another analyze will happen later, probably through autovacuum and wipe out
these values. This is designed to QUICKLY get stats into a table to enable
the database to be operational sooner. This is especially important after
an upgrade/restore, when all stats were wiped out. Other uses could be
adapting this for use the postgres_fdw so that we don't have to do table
sampling on the remote table, and of course statistics injection to test
the query planner.


> 2.
> +they are replaced by the next auto-analyze. This function is used
> by
> +pg_upgrade and pg_restore to
> +convey the statistics from the old system version into the new
> one.
> +   
>
> Is there any demonstration of pg_set_relation_stats and
> pg_set_attribute_stats being used either in pg_upgrade or in
> pg_restore? Perhaps, having them as 0002, 0003 and so on patches might
> show real need for functions like this. It also clarifies how these
> functions pull stats from tables on the old cluster to the tables on
> the new cluster.
>

That code was adapted from do_analyze(), and yes, there is a patch for
pg_dump, but as I noted earlier it is on hold pending feedback.


>
> 3. pg_set_relation_stats and pg_set_attribute_stats seem to be writing
> to pg_class and might affect the plans as stats can get tampered. Can
> we REVOKE the execute permissions from the public out of the box in
> src/backend/catalog/system_functions.sql? This way one can decide who
> to give permissions to.
>

You'd have to be the table owner to alter the stats. I can envision these
functions getting a special role, but they could also be fine as
superuser-only.


>
> 4.
> +SELECT pg_set_relation_stats('stats_export_import.test'::regclass,
> 3.6::float4, 15000);
> + pg_set_relation_stats
> +---
> + t
> +(1 row)
> +
> +SELECT reltuples, relpages FROM pg_class WHERE oid =
> 'stats_export_import.test'::regclass;
> + reltuples | relpages
> +---+--
> +   3.6 |15000
>
> Isn't this test case showing a misuse of these functions? Table
> actually has  no rows, but we are lying to the postgres optimizer on
> stats.


Consider this case. You want to know at what point the query planner will
start using a given index. You can generate dummy data for a thousand, a
million, a billion rows, and wait for that to complete, or you can just
tell the table "I say you have a billion rows, twenty million pages, etc"
and see when it changes.

But again, in most cases, you're setting the values to the same values the
table had on the old database just before the restore/upgrade.


> I think altering stats of a table mustn't be that easy for the
> end user.


Only easy for the end users that happen to be the table owner or a
superuser.


> As mentioned in comment #3, permissions need to be
> tightened. In addition, we can also mark the functions pg_upgrade only
> with CHECK_IS_BINARY_UPGRADE, but that might not work for pg_restore
> (or I don't know if we have a way to know within the server that the
> server is running for pg_restore).
>

I think they will have usage both in postgres_fdw and for tuning.


>
> 5. In continuation to the comment #2, is pg_dump supposed to generate
> pg_set_relation_stats and pg_set_attribute_stats statements for each
> table? When pg_dump does that , pg_restore can automatically load the
> stats.
>

Current plan is to have one TOC entry in the post-data section with a
dependency on the table/index/matview. That let's us leverage existing
filters. The TOC entry will have a series of statements in it, one
pg_set_relation_stats() and one pg_set_attribute_stats() per attribute.


> 9. Isn't it good to add a test case where the plan of a query on table
> after exporting the stats would remain same as that of the original
> table from which the stats are exported? IMO, this is a more realistic
> than just comparing pg_statistic of the tables because this is what an
> end-user wants eventually.
>

I'm sure we can add something like that, but query plan formats change a
lot and are greatly dependent on database configuration, so maintaining
such a test would be a lot of work.

Re: Statistics Import and Export

[Corey Huinker]

>
> Perhaps it's just me ... but it doesn't seem like it's all that many
>
parameters.
>

It's more than I can memorize, so that feels like a lot to me. Clearly it's
not insurmountable.



> > I am a bit concerned about the number of locks on pg_statistic and the
> > relation itself, doing CatalogOpenIndexes/CatalogCloseIndexes once per
> > attribute rather than once per relation. But I also see that this will
> > mostly get used at a time when no other traffic is on the machine, and
> > whatever it costs, it's still faster than the smallest table sample
> (insert
> > joke about "don't have to be faster than the bear" here).
>
> I continue to not be too concerned about this until and unless it's
> actually shown to be an issue.  Keeping things simple and
> straight-forward has its own value.
>

Ok, I'm sold on that plan.


>
> > +/*
> > + * Set statistics for a given pg_class entry.
> > + *
> > + * pg_set_relation_stats(relation Oid, reltuples double, relpages int)
> > + *
> > + * This does an in-place (i.e. non-transactional) update of pg_class,
> just as
> > + * is done in ANALYZE.
> > + *
> > + */
> > +Datum
> > +pg_set_relation_stats(PG_FUNCTION_ARGS)
> > +{
> > + const char *param_names[] = {
> > + "relation",
> > + "reltuples",
> > + "relpages",
> > + };
> > +
> > + Oid relid;
> > + Relationrel;
> > + HeapTuple   ctup;
> > + Form_pg_class   pgcform;
> > +
> > + for (int i = 0; i <= 2; i++)
> > + if (PG_ARGISNULL(i))
> > + ereport(ERROR,
> > +
>  (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > +  errmsg("%s cannot be NULL",
> param_names[i])));
>
> Why not just mark this function as strict..?  Or perhaps we should allow
> NULLs to be passed in and just not update the current value in that
> case?


Strict could definitely apply here, and I'm inclined to make it so.



> Also, in some cases we allow the function to be called with a
> NULL but then make it a no-op rather than throwing an ERROR (eg, if the
> OID ends up being NULL).


Thoughts on it emitting a WARN or NOTICE before returning false?



>   Not sure if that makes sense here or not
> offhand but figured I'd mention it as something to consider.
>
> > + pgcform = (Form_pg_class) GETSTRUCT(ctup);
> > + pgcform->reltuples = PG_GETARG_FLOAT4(1);
> > + pgcform->relpages = PG_GETARG_INT32(2);
>
> Shouldn't we include relallvisible?
>

Yes. No idea why I didn't have that in there from the start.


> Also, perhaps we should use the approach that we have in ANALYZE, and
> only actually do something if the values are different rather than just
> always doing an update.
>

That was how it worked back in v1, more for the possibility that there was
no matching JSON to set values.

Looking again at analyze.c (currently lines 1751-1780), we just check if
there is a row in the way, and if so we replace it. I don't see where we
compare existing values to new values.


>
> > +/*
> > + * Import statistics for a given relation attribute
> > + *
> > + * pg_set_attribute_stats(relation Oid, attname name, stainherit bool,
> > + *stanullfrac float4, stawidth int, stadistinct
> float4,
> > + *stakind1 int2, stakind2 int2, stakind3 int3,
> > + *stakind4 int2, stakind5 int2, stanumbers1
> float4[],
> > + *stanumbers2 float4[], stanumbers3 float4[],
> > + *stanumbers4 float4[], stanumbers5 float4[],
> > + *stanumbers1 float4[], stanumbers2 float4[],
> > + *stanumbers3 float4[], stanumbers4 float4[],
> > + *stanumbers5 float4[], stavalues1 text,
> > + *stavalues2 text, stavalues3 text,
> > + *stavalues4 text, stavalues5 text);
> > + *
> > + *
> > + */
>
> Don't know that it makes sense to just repeat the function declaration
> inside a comment like this- it'll just end up out of date.
>

Historical artifact - previous versions had a long explanation of the JSON
format.



>
> > +Datum
> > +pg_set_attribute_stats(PG_FUNCTION_ARGS)
>
> > + /* names of columns that cannot be null */
> > + const char *required_param_names[] = {
> > + "relation",
> > + "attname",
> > + "stainherit",
> > + "stanullfrac",
> > + "stawidth",
> > + "stadistinct",
> > + "stakind1",
> > + "stakind2",
> > + "stakind3",
> > + "stakind4",
> > + "stakind5",
> > + };
>
> Same comment here as above wrt NULL being passed in.
>

In this case, the last 10 params (stanumbersN and stavaluesN) can be null,
and are NULL more often than not.


>
> > + for (int k = 0; k < 5; k++)
>
> Shouldn't we use STATISTIC_NUM_SLOTS here?
>

Yes, I 

Re: Statistics Import and Export

[Corey Huinker]

>
>
>
> Having some discussion around that would be useful.  Is it better to
> have a situation where there are stats for some columns but no stats for
> other columns?  There would be a good chance that this would lead to a
> set of queries that were properly planned out and a set which end up
> with unexpected and likely poor query plans due to lack of stats.
> Arguably that's better overall, but either way an ANALYZE needs to be
> done to address the lack of stats for those columns and then that
> ANALYZE is going to blow away whatever stats got loaded previously
> anyway and all we did with a partial stats load was maybe have a subset
> of queries have better plans in the interim, after having expended the
> cost to try and individually load the stats and dealing with the case of
> some of them succeeding and some failing.
>

It is my (incomplete and entirely second-hand) understanding is that
pg_upgrade doesn't STOP autovacuum, but sets a delay to a very long value
and then resets it on completion, presumably because analyzing a table
before its data is loaded and indexes are created would just be a waste of
time.



>
> Overall, I'd suggest we wait to see what Corey comes up with in terms of
> doing the stats load for all attributes in a single function call,
> perhaps using the VALUES construct as you suggested up-thread, and then
> we can contemplate if that's clean enough to work or if it's so grotty
> that the better plan would be to do per-attribute function calls.  If it
> ends up being the latter, then we can revisit this discussion and try to
> answer some of the questions raised above.
>

In the patch below, I ended up doing per-attribute function calls, mostly
because it allowed me to avoid creating a custom data type for the portable
version of pg_statistic. This comes at the cost of a very high number of
parameters, but that's the breaks.

I am a bit concerned about the number of locks on pg_statistic and the
relation itself, doing CatalogOpenIndexes/CatalogCloseIndexes once per
attribute rather than once per relation. But I also see that this will
mostly get used at a time when no other traffic is on the machine, and
whatever it costs, it's still faster than the smallest table sample (insert
joke about "don't have to be faster than the bear" here).

This raises questions about whether a failure in one attribute update
statement should cause the others in that relation to roll back or not, and
I can see situations where both would be desirable.

I'm putting this out there ahead of the pg_dump / fe_utils work, mostly
because what I do there heavily depends on how this is received.

Also, I'm still seeking confirmation that I can create a pg_dump TOC entry
with a chain of commands (e.g. BEGIN; ...  COMMIT; ) or if I have to fan
them out into multiple entries.

Anyway, here's v7. Eagerly awaiting feedback.
From 9bc7200b8a67efefe20453e0c48aed8a0a5f62f8 Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Thu, 7 Mar 2024 22:18:48 -0500
Subject: [PATCH v7] Create pg_set_relation_stats, pg_set_attribute_stats.

These functions will be used by pg_dump/restore and pg_upgrade to convey
relation and attribute statistics from the source database to the
target. This would be done instead of vacuumdb --analyze-in-stages.

Both functions take an oid to identify the target relation that will
receive the statistics. There is nothing requiring that relation to be
the same one as the one exported, though the statistics would have to
make sense in the context of the new relation. Typecasts for stavaluesN
parameters may fail if the destination column is not of the same type as
the source column.

The parameters of pg_set_attribute_stats identify the attribute by name
rather than by attnum. This is intentional because the column order may
be different in situations other than binary upgrades. For example,
dropped columns do not carry over in a restore.

The statistics imported by pg_set_attribute_stats are imported
transactionally like any other operation. However, pg_set_relation_stats
does it's update in-place, which is to say non-transactionally. This is
in line with what ANALYZE does to avoid table bloat in pg_class.

These functions also allows for tweaking of table statistics in-place,
allowing the user to inflate rowcounts, skew histograms, etc, to see
what those changes will evoke from the query planner.
---
 src/include/catalog/pg_proc.dat   |  19 +-
 src/include/statistics/statistics.h   |   3 +
 src/backend/statistics/Makefile   |   3 +-
 src/backend/statistics/meson.build|   1 +
 src/backend/statistics/statistics.c   | 360 ++
 .../regress/expected/stats_export_import.out  | 211 ++
 src/test/regress/parallel_schedule|   2 +-
 src/test/regress/sql/stats_export_import.sql  | 198 ++
 doc/src/sgml/func.sgml   

Re: Statistics Import and Export

[Corey Huinker]

>
>
> > BEGIN;
> > LOCK TABLE schema.relation IN SHARE UPDATE EXCLUSIVE MODE;
> > LOCK TABLE pg_catalog.pg_statistic IN ROW UPDATE EXCLUSIVE MODE;
> > SELECT pg_import_rel_stats('schema.relation', ntuples, npages);
> > SELECT pg_import_pg_statistic('schema.relation', 'id', ...);
> > SELECT pg_import_pg_statistic('schema.relation', 'name', ...);
>
> How well would this simplify to the following:
>
> SELECT pg_import_statistic('schema.relation', attname, ...)
> FROM (VALUES ('id', ...), ...) AS relation_stats (attname, ...);
>
> Or even just one VALUES for the whole statistics loading?
>

I'm sorry, I don't quite understand what you're suggesting here. I'm about
to post the new functions, so perhaps you can rephrase this in the context
of those functions.


> I suspect the main issue with combining this into one statement
> (transaction) is that failure to load one column's statistics implies
> you'll have to redo all the other statistics (or fail to load the
> statistics at all), which may be problematic at the scale of thousands
> of relations with tens of columns each.


Yes, that is is a concern, and I can see value to having it both ways (one
failure fails the whole table's worth of set_something() functions, but I
can also see emitting a warning instead of error and returning false. I'm
eager to get feedback on which the community would prefer, or perhaps even
make it a parameter.

Re: Statistics Import and Export

[Corey Huinker]

>
>
> That’s certainly a fair point and my initial reaction (which could
> certainly be wrong) is that it’s unlikely to be an issue- but also, if you
> feel you could make it work with an array and passing all the attribute
> info in with one call, which I suspect would be possible but just a bit
> more complex to build, then sure, go for it. If it ends up being overly
> unwieldy then perhaps the  per-attribute call would be better and we could
> perhaps acquire the lock before the function calls..?  Doing a check to see
> if we have already locked it would be cheaper than trying to acquire a new
> lock, I’m fairly sure.
>

Well the do_analyze() code was already ok with acquiring the lock once for
non-inherited stats and again for inherited stats, so the locks were
already not the end of the world. However, that's at most a 2x of the
locking required, and this would natts * x, quite a bit more. Having the
procedures check for a pre-existing lock seems like a good compromise.


> Also per our prior discussion- this makes sense to include in post-data
> section, imv, and also because then we have the indexes we may wish to load
> stats for, but further that also means it’ll be in the paralleliziable part
> of the process, making me a bit less concerned overall about the individual
> timing.
>

The ability to parallelize is pretty persuasive. But is that per-statement
parallelization or do we get transaction blocks? i.e. if we ended up
importing stats like this:

BEGIN;
LOCK TABLE schema.relation IN SHARE UPDATE EXCLUSIVE MODE;
LOCK TABLE pg_catalog.pg_statistic IN ROW UPDATE EXCLUSIVE MODE;
SELECT pg_import_rel_stats('schema.relation', ntuples, npages);
SELECT pg_import_pg_statistic('schema.relation', 'id', ...);
SELECT pg_import_pg_statistic('schema.relation', 'name', ...);
SELECT pg_import_pg_statistic('schema.relation', 'description', ...);
...
COMMIT;

Re: Statistics Import and Export

[Corey Huinker]

>
>
>
> Having looked through this thread and discussed a bit with Corey
> off-line, the approach that Tom laid out up-thread seems like it would
> make the most sense overall- that is, eliminate the JSON bits and the
> SPI and instead export the stats data by running queries from the new
> version of pg_dump/server (in the FDW case) against the old server
> with the intelligence of how to transform the data into the format
> needed for the current pg_dump/server to accept, through function calls
> where the function calls generally map up to the rows/information being
> updated- a call to update the information in pg_class for each relation
> and then a call for each attribute to update the information in
> pg_statistic.
>

Thanks for the excellent summary of our conversation, though I do add that
we discussed a problem with per-attribute functions: each function would be
acquiring locks on both the relation (so it doesn't go away) and
pg_statistic, and that lock thrashing would add up. Whether that overhead
is judged significant or not is up for discussion. If it is significant, it
makes sense to package up all the attributes into one call, passing in an
array of some new pg_statistic-esque special typethe very issue that
sent me down the JSON path.

I certainly see the flexibility in having a per-attribute functions, but am
concerned about non-binary-upgrade situations where the attnums won't line
up, and if we're passing them by name then the function has dig around
looking for the right matching attnum, and that's overhead too. In the
whole-table approach, we just iterate over the attributes that exist, and
find the matching parameter row.

Re: Document efficient self-joins / UPDATE LIMIT techniques.

[Corey Huinker]

>
> > As for whether it's commonplace, when I was a consultant I had a number
> > of customers that I had who bemoaned how large updates caused big
> > replica lag, basically punishing access to records they did care about
> > in order to properly archive or backfill records they don't care about.
> > I used the technique a lot, putting the update/delete in a loop, and
> > often running multiple copies of the same script at times when I/O
> > contention was low, but if load levels rose it was trivial to just kill
> > a few of the scripts until things calmed down.
>
> I've also used the technique quite a lot, but only using the PK,
> didn't know about the ctid trick, so many thanks for documenting it.


tid-scans only became a thing a few versions ago (12?). Prior to that, PK
was the only way to go.

Re: Document efficient self-joins / UPDATE LIMIT techniques.

[Corey Huinker]

On Tue, Feb 13, 2024 at 11:51 AM Joel Jacobson  wrote:

> On Tue, Feb 13, 2024, at 10:28, Laurenz Albe wrote:
> > On Mon, 2024-02-12 at 12:24 -0500, Corey Huinker wrote:
> >> > Do you plan to add it to the commitfest?  If yes, I'd set it "ready
> for committer".
> >>
> >> Commitfest entry reanimated.
> >
> > Truly... you created a revenant in the already closed commitfest.
> >
> > I closed that again and added a new entry in the open commitfest.
> >
> > Yours,
> > Laurenz Albe
>
> This thread reminded me of the old discussion "LIMIT for UPDATE and
> DELETE" from 2014 [1].
>
> Back in 2014, it was considered a "fringe feature" by some. It is thought
> to be more commonplace today?
>
> /Joel
>
> [1]
> https://www.postgresql.org/message-id/flat/CADB9FDf-Vh6RnKAMZ4Rrg_YP9p3THdPbji8qe4qkxRuiOwm%3Dmg%40mail.gmail.com


This patch came out of a discussion at the last PgCon with the person who
made the "fringe feature" quote, who seemed quite supportive of documenting
the technique. The comment may have been in regards to actually
implementing a LIMIT clause on UPDATE and DELETE, which isn't in the SQL
standard and would be difficult to implement as the two statements have no
concept of ordering. Documenting the workaround would alleviate some
interest in implementing a nonstandard feature.

As for whether it's commonplace, when I was a consultant I had a number of
customers that I had who bemoaned how large updates caused big replica lag,
basically punishing access to records they did care about in order to
properly archive or backfill records they don't care about. I used the
technique a lot, putting the update/delete in a loop, and often running
multiple copies of the same script at times when I/O contention was low,
but if load levels rose it was trivial to just kill a few of the scripts
until things calmed down.

Re: Statistics Import and Export

[Corey Huinker]

>
> Also, it says "statistics are replaced" but it's quite clear if that
> applies only to matching statistics or if all stats are deleted first
> and then the new stuff is inserted. (FWIW remove_pg_statistics clearly
> deletes all pre-existing stats).
>

All are now deleted first, both in the pg_statistic and
pg_statistic_ext_data tables. The previous version was taking a more
"replace it if we find a new value" approach, but that's overly
complicated, so following the example set by extended statistics seemed
best.



> - import_pg_statistics: I somewhat dislike that we're passing arguments
> as datum[] array - it's hard to say what the elements are expected to
> be, etc. Maybe we should expand this, to make it clear. How do we even
> know the array is large enough?
>

Completely fair. Initially that was done with the expectation that the
array would be the same for both regular stats and extended stats, but that
was no longer the case.


> - I don't quite understand why we need examine_rel_attribute. It sets a
> lot of fields in the VacAttrStats struct, but then we only use attrtypid
> and attrtypmod from it - so why bother and not simply load just these
> two fields? Or maybe I miss something.
>

I think you're right, we don't need it anymore for regular statistics. We
still need it in extended stats because statext_store() takes a subset of
the vacattrstats rows as an input.

Which leads to a side issue. We currently have 3 functions:
examine_rel_attribute and the two varieties of examine_attribute (one in
analyze.c and the other in extended stats). These are highly similar
but just different enough that I didn't feel comfortable refactoring them
into a one-size-fits-all function, and I was particularly reluctant to
modify existing code for the ANALYZE path.


>
> - examine_rel_attribute can return NULL, but get_attrinfo does not check
> for NULL and just dereferences the pointer. Surely that can lead to
> segfaults?
>

Good catch, and it highlights how little we need VacAttrStats for regular
statistics.


>
> - validate_no_duplicates and the other validate functions would deserve
> a better docs, explaining what exactly is checked (it took me a while to
> realize we check just for duplicates), what the parameters do etc.
>

Those functions are in a fairly formative phase - I expect a conversation
about what sort of validations we want to do to ensure that the statistics
being imported make sense, and under what circumstances we would forego
some of those checks.


>
> - Do we want to make the validate_ functions part of the public API? I
> realize we want to use them from multiple places (regular and extended
> stats), but maybe it'd be better to have an "internal" header file, just
> like we have extended_stats_internal?
>

I see no need to have them be a part of the public API. Will move.


>
> - I'm not sure we do "\set debug f" elsewhere. It took me a while to
> realize why the query outputs are empty ...
>

That was an experiment that rose out of the difficulty in determining
_where_ a difference was when the set-difference checks failed. So far I
like it, and I'm hoping it catches on.



>
>
> 0002
>
> - I'd rename create_stat_ext_entry to statext_create_entry.
>
> - Do we even want to include OIDs from the source server? Why not to
> just have object names and resolve those? Seems safer - if the target
> server has the OID allocated to a different object, that could lead to
> confusing / hard to detect issues.
>

The import functions would obviously never use the imported oids to look up
objects on the destination system. Rather, they're there to verify that the
local object oid matches the exported object oid, which is true in the case
of a binary upgrade.

The export format is an attempt to export the pg_statistic[_ext_data] for
that object as-is, and, as Tom suggested, let the import function do the
transformations. We can of course remove them if they truly have no purpose
for validation.


>
> - What happens if we import statistics which includes data for extended
> statistics object which does not exist on the target machine?
>

The import function takes an oid of the object (relation or extstat
object), and the json payload is supposed to be the stats for ONE
corresponding object. Multiple objects of data really don't fit into the
json format, and statistics exported for an object that does not exist on
the destination system would have no meaningful invocation. I envision the
dump file looking like this

CREATE TABLE public.foo ();

SELECT pg_import_rel_stats('public.foo'::regclass, , option
flag, option flag);

So a call against a nonexistent object would fail on the regclass cast.


>
> - pg_import_ext_stats seems to not use require_match_oids - bug?
>

I haven't yet seen a good way to make use of matching oids in extended
stats. Checking matching operator/collation oids would make sense, but
little else.


>
>
> 0003
>
> - no SGML docs for the new tools?
>

Correct. I 

Re: Document efficient self-joins / UPDATE LIMIT techniques.

[Corey Huinker]

>
> Do you plan to add it to the commitfest?  If yes, I'd set it "ready for
> committer".
>
> Commitfest entry reanimated.

Re: Document efficient self-joins / UPDATE LIMIT techniques.

[Corey Huinker]

>
>
> - About the style: there is usually an empty line between an ending 
>   and the next starting .  It does not matter for correctness, but I
>   think it makes the source easier to read.
>

Done. I've seen them with spaces and without, and have no preference.


>
> - I would rather have only "here" as link text rather than "in greater
> details
>   here".  Even better would be something that gives the reader a clue where
>   the link will take her, like
>   the documentation of
> UPDATE.
>

Done.

>
> - I am not sure if it is necessary to have the  at all.
>   I'd say that it is just a trivial variation of the UPDATE example.
>   On the other hand, a beginner might find the example useful.
>   Not sure.
>

I think a beginner would find it useful. The join syntax for DELETE is
different from UPDATE in a way that has never made sense to me, and a
person with only the UPDATE example might try just replacing UPDATE WITH
DELETE and eliminating the SET clause, and frustration would follow. We
have an opportunity to show the equivalent join in both cases, let's use it.



> I think the "in" before between is unnecessary and had better be removed,
> but
> I'll defer to the native speaker.
>

The "in" is more common when spoken. Removed.
From a6b57bf3a88c5df614b5dede99af3e99fe8e8089 Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Mon, 12 Feb 2024 11:32:49 -0500
Subject: [PATCH v3] Documentation: Show alternatives to LIMIT on UPDATE and
 DELETE

Show examples of how to simulate UPDATE or DELETE with a LIMIT clause.

These examples also serve to show the existence and utility of ctid self-joins.
---
 doc/src/sgml/ref/delete.sgml | 18 ++
 doc/src/sgml/ref/update.sgml | 37 
 2 files changed, 55 insertions(+)

diff --git a/doc/src/sgml/ref/delete.sgml b/doc/src/sgml/ref/delete.sgml
index 1b81b4e7d7..1544a28e18 100644
--- a/doc/src/sgml/ref/delete.sgml
+++ b/doc/src/sgml/ref/delete.sgml
@@ -234,6 +234,24 @@ DELETE FROM films
In some cases the join style is easier to write or faster to
execute than the sub-select style.
   
+  
+   While there is no LIMIT clause for
+   DELETE, it is possible to get a similar effect
+   using the method for UPDATE operations described
+   the documentation of UPDATE.
+
+WITH delete_batch AS (
+  SELECT l.ctid
+  FROM user_logs AS l
+  WHERE l.status = 'archived'
+  ORDER BY l.creation_date
+  LIMIT 1
+  FOR UPDATE
+)
+DELETE FROM user_logs AS ul
+USING delete_branch AS del
+WHERE ul.ctid = del.ctid;
+
  
 
  
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 2ab24b0523..ed3dd029c7 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -442,6 +442,43 @@ COMMIT;
 
 UPDATE films SET kind = 'Dramatic' WHERE CURRENT OF c_films;
 
+  
+   Updates affecting many rows can have negative effects on system performance,
+   such as table bloat, increased replica lag, increased lock contention,
+   and possible failure of the operation due to a deadlock. In such situations
+   it can make sense to perform the operation in smaller batches. Performing a
+   VACUUM operation on the table between batches can help
+   reduce table bloat. The
+   SQL standard does
+   not define a LIMIT clause for UPDATE
+   operations, but it is possible get a similar effect through the use of a
+   Common Table Expression and an
+   efficient self-join via the system column
+   ctid:
+
+WITH exceeded_max_retries AS (
+  SELECT w.ctid
+  FROM work_item AS w
+  WHERE w.status = 'active'
+  AND w.num_retries > 10
+  ORDER BY w.retry_timestamp
+  FOR UPDATE
+  LIMIT 5000
+)
+UPDATE work_item
+SET status = 'failed'
+FROM exceeded_max_retries AS emr
+WHERE work_item.ctid = emr.ctid
+
+If lock contention is a concern, then SKIP LOCKED can
+be added to the CTE. However, one final
+UPDATE without SKIP LOCKED or
+LIMIT will be needed to ensure that no matching rows
+were overlooked. The use of an ORDER BY clause allows
+the command to prioritize which rows will be locked and updated. This can
+also reduce contention with other update operations if they use the same
+ordering.
+  
  
 
  
-- 
2.43.0

Re: Document efficient self-joins / UPDATE LIMIT techniques.

[Corey Huinker]

>
> I have changed the status of commitfest entry to "Returned with
> Feedback" as Laurenz's comments have not yet been resolved. Please
> handle the comments and update the commitfest entry accordingly.
>
>
Here's another attempt, applying Laurenz's feedback:

I removed all changes to the SELECT documentation. That might seem strange
given that the heavy lifting happens in the SELECT, but I'm working from
the assumption that people's greatest need for a ctid self-join will be
because they are trying to find the LIMIT keyword on UPDATE/DELETE and
coming up empty.

Because the join syntax is subtly different between UPDATE and DELETE, I've
kept code examples in both, but the detailed explanation is in UPDATE under
the anchor "update-limit" and the DELETE example links to it.
From 298c812838491408e6910f7535067ea147abe5fc Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Sat, 3 Feb 2024 14:38:50 -0500
Subject: [PATCH v2] Documentation: Show alternatives to LIMIT on UPDATE and
 DELETE

Show examples of how to simulate UPDATE or DELETE with a LIMIT clause.

These examples also serve to show the existence and utility of ctid self-joins.
---
 doc/src/sgml/ref/delete.sgml | 18 +
 doc/src/sgml/ref/update.sgml | 38 +++-
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/delete.sgml b/doc/src/sgml/ref/delete.sgml
index 1b81b4e7d7..21aae30e91 100644
--- a/doc/src/sgml/ref/delete.sgml
+++ b/doc/src/sgml/ref/delete.sgml
@@ -234,6 +234,24 @@ DELETE FROM films
In some cases the join style is easier to write or faster to
execute than the sub-select style.
   
+  
+   While there is no LIMIT clause for
+   DELETE, it is possible to get a similar effect
+   using the method for UPDATE operations described
+   in greater detail here.
+
+WITH delete_batch AS (
+  SELECT l.ctid
+  FROM user_logs AS l
+  WHERE l.status = 'archived'
+  ORDER BY l.creation_date
+  LIMIT 1
+  FOR UPDATE
+)
+DELETE FROM user_logs AS ul
+USING delete_branch AS del
+WHERE ul.ctid = del.ctid;
+
  
 
  
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 2ab24b0523..49e0dc29de 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -434,7 +434,6 @@ UPDATE wines SET stock = stock + 24 WHERE winename = 'Chateau Lafite 2003';
 COMMIT;
 
   
-
   
Change the kind column of the table
films in the row on which the cursor
@@ -442,6 +441,43 @@ COMMIT;
 
 UPDATE films SET kind = 'Dramatic' WHERE CURRENT OF c_films;
 
+  
+   Updates affecting many rows can have negative effects on system performance,
+   such as table bloat, increased replica lag, increased lock contention,
+   and possible failure of the operation due to a deadlock. In such situations
+   it can make sense to perform the operation in smaller batches. Performing a
+   VACUUM operation on the table in between batches can help
+   reduce table bloat. The
+   SQL standard does
+   not define a LIMIT clause for UPDATE
+   operations, but it is possible get a similar effect through the use of a
+   Common Table Expression and an
+   efficient self-join via the system column
+   ctid:
+
+WITH exceeded_max_retries AS (
+  SELECT w.ctid
+  FROM work_item AS w
+  WHERE w.status = 'active'
+  AND w.num_retries > 10
+  ORDER BY w.retry_timestamp
+  FOR UPDATE
+  LIMIT 5000
+)
+UPDATE work_item
+SET status = 'failed'
+FROM exceeded_max_retries AS emr
+WHERE work_item.ctid = emr.ctid
+
+If lock contention is a concern, then SKIP LOCKED can
+be added to the CTE. However, one final
+UPDATE without SKIP LOCKED or
+LIMIT will be needed to ensure that no matching rows
+were overlooked. The use of an ORDER BY clause allows
+the command to prioritize which rows will be locked and updated. This can
+also reduce contention with other update operations if they use the same
+ordering.
+  
  
 
  
-- 
2.43.0

Re: Statistics Import and Export

[Corey Huinker]

On Mon, Jan 22, 2024 at 1:09 AM Peter Smith  wrote:

> 2024-01 Commitfest.
>
> Hi, This patch has a CF status of "Needs Review" [1], but it seems
> there were CFbot test failures last time it was run [2]. Please have a
> look and post an updated version if necessary.
>
> ==
> [1] https://commitfest.postgresql.org/46/4538/
> [2]
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/4538
>
> Kind Regards,
> Peter Smith.
>

Attached is v4 of the statistics export/import patch.

This version has been refactored to match the design feedback received
previously.

The system views are gone. These were mostly there to serve as a baseline
for what an export query would look like. That role is temporarily
reassigned to pg_export_stats.c, but hopefully they will be integrated into
pg_dump in the next version. The regression test also contains the version
of each query suitable for the current server version.

The export format is far closer to the raw format of pg_statistic and
pg_statistic_ext_data, respectively. This format involves exporting oid
values for types, collations, operators, and attributes - values which are
specific to the server they were created on. To make sense of those values,
a subset of the columns of pg_type, pg_attribute, pg_collation, and
pg_operator are exported as well, which allows pg_import_rel_stats() and
pg_import_ext_stats() to reconstitute the data structure as it existed on
the old server, and adapt it to the modern structure and local schema
objects.

pg_import_rel_stats matches up local columns with the exported stats by
column name, not attnum. This allows for stats to be imported when columns
have been dropped, added, or reordered.

pg_import_ext_stats can also handle column reordering, though it currently
would get confused by changes in expressions that maintain the same result
data type. I'm not yet brave enough to handle importing nodetrees, nor do I
think it's wise to try. I think we'd be better off validating that the
destination extended stats object is identical in structure, and to fail
the import of that one object if it isn't perfect.

Export formats go back to v10.

Re: Statistics Import and Export

[Corey Huinker]

>
> But maybe it's enough to just do what you did - if we get an MCELEM
> slot, can it ever contain anything else than array of elements of the
> attribute array type? I'd bet that'd cause all sorts of issues, no?
>
>
Thanks for the explanation of why it wasn't working for me. Knowing that
the case of MCELEM + is-array-type is the only case where we'd need to do
that puts me at ease.

Re: Statistics Import and Export

[Corey Huinker]

On Wed, Dec 27, 2023 at 10:10 PM Tom Lane  wrote:

> Corey Huinker  writes:
> > Export functions was my original plan, for simplicity, maintenance, etc,
> > but it seemed like I'd be adding quite a few functions, so the one view
> > made more sense for an initial version. Also, I knew that pg_dump or some
> > other stats exporter would have to inline the guts of those functions
> into
> > queries for older versions, and adapting a view definition seemed more
> > straightforward for the reader than function definitions.
>
> Hmm, I'm not sure we are talking about the same thing at all.
>

Right, I was conflating two things.


>
> What I am proposing is *import* functions.  I didn't say anything about
> how pg_dump obtains the data it prints; however, I would advocate that
> we keep that part as simple as possible.  You cannot expect export
> functionality to know the requirements of future server versions,
> so I don't think it's useful to put much intelligence there.
>

True, but presumably you'd be using the pg_dump/pg_upgrade of that future
version to do the exporting, so the export format would always be tailored
to the importer's needs.


>
> So I think pg_dump should produce a pretty literal representation of
> what it finds in the source server's catalog, and then rely on the
> import functions in the destination server to make sense of that
> and do whatever slicing-n-dicing is required.
>

Obviously it can't be purely literal, as we have to replace the oid values
with whatever text representation we feel helps us carry forward. In
addition, we're setting the number of tuples and number of pages directly
in pg_class, and doing so non-transactionally just like ANALYZE does. We
could separate that out into its own import function, but then we're
locking every relation twice, once for the tuples/pages and once again for
the pg_statistic import.

My current line of thinking was that the stats import call, if enabled,
would immediately follow the CREATE statement of the object itself, but
that requires us to have everything we need to know for the import passed
into the import function, so we'd be needing a way to serialize _that_. If
you're thinking that we have one big bulk stats import, that might work,
but it also means that we're less tolerant of failures in the import step.

Re: Statistics Import and Export

[Corey Huinker]

>
> Yeah, this is pretty much what I meant by "functional" interface. But if
> I said maybe the format implemented by the patch is maybe too close to
> how we store the statistics, then this has exactly the same issue. And
> it has other issues too, I think - it breaks down the stats into
> multiple function calls, so ensuring the sanity/correctness of whole
> sets of statistics gets much harder, I think.
>

Export functions was my original plan, for simplicity, maintenance, etc,
but it seemed like I'd be adding quite a few functions, so the one view
made more sense for an initial version. Also, I knew that pg_dump or some
other stats exporter would have to inline the guts of those functions into
queries for older versions, and adapting a view definition seemed more
straightforward for the reader than function definitions.

Re: Statistics Import and Export

[Corey Huinker]

>
>
>   As mentioned already, we'd also need some sort of
> version identifier, and we'd expect the load_statistics() functions
> to be able to transform the data if the old version used a different
> representation.  I agree with the idea that an explicit representation
> of the source table attribute's type would be wise, too.


There is a version identifier currently (its own column not embedded in the
JSON), but I discovered that I was able to put the burden on the export
queries to spackle-over the changes in the table structures over time.
Still, I knew that we'd need the version number in there eventually.

Re: Statistics Import and Export

[Corey Huinker]

On Mon, Dec 25, 2023 at 8:18 PM Tomas Vondra 
wrote:

> Hi,
>
> I finally had time to look at the last version of the patch, so here's a
> couple thoughts and questions in somewhat random order. Please take this
> as a bit of a brainstorming and push back if you disagree some of my
> comments.
>
> In general, I like the goal of this patch - not having statistics is a
> common issue after an upgrade, and people sometimes don't even realize
> they need to run analyze. So, it's definitely worth improving.
>
> I'm not entirely sure about the other use case - allowing people to
> tweak optimizer statistics on a running cluster, to see what would be
> the plan in that case. Or more precisely - I agree that would be an
> interesting and useful feature, but maybe the interface should not be
> the same as for the binary upgrade use case?
>
>
> interfaces
> --
>
> When I thought about the ability to dump/load statistics in the past, I
> usually envisioned some sort of DDL that would do the export and import.
> So for example we'd have EXPORT STATISTICS / IMPORT STATISTICS commands,
> or something like that, and that'd do all the work. This would mean
> stats are "first-class citizens" and it'd be fairly straightforward to
> add this into pg_dump, for example. Or at least I think so ...
>
> Alternatively we could have the usual "functional" interface, with a
> functions to export/import statistics, replacing the DDL commands.
>
> Unfortunately, none of this works for the pg_upgrade use case, because
> existing cluster versions would not support this new interface, of
> course. That's a significant flaw, as it'd make this useful only for
> upgrades of future versions.
>

This was the reason I settled on the interface that I did: while we can
create whatever interface we want for importing the statistics, we would
need to be able to extract stats from databases using only the facilities
available in those same databases, and then store that in a medium that
could be conveyed across databases, either by text files or by saving them
off in a side table prior to upgrade. JSONB met the criteria.


>
> So I think for the pg_upgrade use case, we don't have much choice other
> than using "custom" export through a view, which is what the patch does.
>
> However, for the other use case (tweaking optimizer stats) this is not
> really an issue - that always happens on the same instance, so no issue
> with not having the "export" function and so on. I'd bet there are more
> convenient ways to do this than using the export view. I'm sure it could
> share a lot of the infrastructure, ofc.
>

So, there is a third use case - foreign data wrappers. When analyzing a
foreign table, at least one in the postgresql_fdw family of foreign
servers, we should be able to send a query specific to the version and
dialect of that server, get back the JSONB, and import those results. That
use case may be more tangible to you than the tweak/tuning case.


>
>
>
> JSON format
> ---
>
> As for the JSON format, I wonder if we need that at all? Isn't that an
> unnecessary layer of indirection? Couldn't we simply dump pg_statistic
> and pg_statistic_ext_data in CSV, or something like that? The amount of
> new JSONB code seems to be very small, so it's OK I guess.
>

I see a few problems with dumping pg_statistic[_ext_data]. The first is
that the importer now has to understand all of the past formats of those
two tables. The next is that the tables are chock full of Oids that don't
necessarily carry forward. I could see us having a text-ified version of
those two tables, but we'd need that for all previous iterations of those
table formats. Instead, I put the burden on the stats export to de-oid the
data and make it *_in() function friendly.


> That's pretty much why I envisioned a format "grouping" the arrays for a
> particular type of statistics (MCV, histogram) into the same object, as
> for example in
>
>  {
>"mcv" : {"values" : [...], "frequencies" : [...]}
>"histogram" : {"bounds" : [...]}
>  }
>

I agree that would be a lot more readable, and probably a lot more
debuggable. But I went into this unsure if there could be more than one
stats slot of a given kind per table. Knowing that they must be unique
helps.


> But that's probably much harder to generate from plain SQL (at least I
> think so, I haven't tried).
>

I think it would be harder, but far from impossible.



> data missing in the export
> --
>
> I think the data needs to include more information. Maybe not for the
> pg_upgrade use case, where it's mostly guaranteed not to change, but for
> the "manual tweak" use case it can change. And I don't think we want two
> different formats - we want one, working for everything.
>

I"m not against this at all, and I started out doing that, but the
qualified names of operators got _ugly_, and I quickly realized that what I
was generating wouldn't matter, either the input data would make sense for

Re: Statistics Import and Export

[Corey Huinker]

On Fri, Dec 15, 2023 at 3:36 AM Andrei Lepikhov 
wrote:

> On 13/12/2023 17:26, Corey Huinker wrote:> 4. I don't yet have a
> complete vision for how these tools will be used
> > by pg_upgrade and pg_dump/restore, the places where these will provide
> > the biggest win for users.
>
> Some issues here with docs:
>
> func.sgml:28465: parser error : Opening and ending tag mismatch: sect1
> line 26479 and sect2
>
>^
>

Apologies, will fix.


>
> Also, as I remember, we already had some attempts to invent dump/restore
> statistics [1,2]. They were stopped with the problem of type
> verification. What if the definition of the type has changed between the
> dump and restore? As I see in the code, Importing statistics you just
> check the column name and don't see into the type.
>

We look up the imported statistics via column name, that is correct.

However, the values in stavalues and mcv and such are stored purely as
text, so they must be casted using the input functions for that particular
datatype. If that column definition changed, or the underlying input
function changed, the stats import of that particular table would fail. It
should be noted, however, that those same input functions were used to
bring the data into the table via restore, so it would have already failed
on that step. Either way, the structure of the table has effectively
changed, so failure to import those statistics would be a good thing.


>
> [1] Backup and recovery of pg_statistic
> https://www.postgresql.org/message-id/flat/724322880.K8vzik8zPz%40abook


That proposal sought to serialize enough information on the old server such
that rows could be directly inserted into pg_statistic on the new server.
As was pointed out at the time, version N of a server cannot know what the
format of pg_statistic will be in version N+1.

This patch avoids that problem by inspecting the structure of the object to
be faux-analyzed, and using that to determine what parts of the JSON to
fetch, and what datatype to cast values to in cases like mcv and
stavaluesN. The exported JSON has no oids in it whatseover, all elements
subject to casting on import have already been cast to text, and the record
returned has the server version number of the producing system, and the
import function can use that to determine how it interprets the data it
finds.


>
> [2] Re: Ideas about a better API for postgres_fdw remote estimates
>
> https://www.postgresql.org/message-id/7a40707d-1758-85a2-7bb1-6e5775518e64%40postgrespro.ru
>
>
This one seems to be pulling oids from the remote server, and we can't
guarantee their stability across systems, especially for objects and
operators from extensions. I tried to go the route of extracting the full
text name of an operator, but discovered that the qualified names, in
addition to being unsightly, were irrelevant because we can't insert stats
that disagree about type with the attribute/expression. So it didn't matter
what type the remote system thought it had, the local system was going to
coerce it into the expected data type or ereport() trying.

I think there is hope for having do_analyze() run a remote query fetching
the remote table's exported stats and then storing them locally, possibly
after some modification, and that would save us from having to sample a
remote table.

Re: Statistics Import and Export

[Corey Huinker]

>
>
>
> Maybe I just don't understand, but I'm pretty sure ANALYZE does not
> derive index stats from column stats. It actually builds them from the
> row sample.
>

That is correct, my error.


>
> > * now support extended statistics except for MCV, which is currently
> > serialized as an difficult-to-decompose bytea field.
>
> Doesn't pg_mcv_list_items() already do all the heavy work?
>

Thanks! I'll look into that.

The comment below in mcv.c made me think there was no easy way to get
output.

/*
 * pg_mcv_list_out  - output routine for type pg_mcv_list.
 *
 * MCV lists are serialized into a bytea value, so we simply call byteaout()
 * to serialize the value into text. But it'd be nice to serialize that into
 * a meaningful representation (e.g. for inspection by people).
 *
 * XXX This should probably return something meaningful, similar to what
 * pg_dependencies_out does. Not sure how to deal with the deduplicated
 * values, though - do we want to expand that or not?
 */

Re: Document efficient self-joins / UPDATE LIMIT techniques.

[Corey Huinker]

>
>
> I think the SQL statements should end with semicolons.  Our SQL examples
> are usually written like that.
>

ok



>
> Our general style with CTEs seems to be (according to
> https://www.postgresql.org/docs/current/queries-with.html):
>
>  WITH quaxi AS (
>  SELECT ...
>  )
>  SELECT ...;
>

done


>
> About the DELETE example:
> -
>
> The text suggests that a single, big DELETE operation can consume
> too many resources.  That may be true, but the sum of your DELETEs
> will consume even more resources.
>
> In my experience, the bigger problem with bulk deletes like that is
> that you can run into deadlocks easily, so maybe that would be a
> better rationale to give.  You could say that with this technique,
> you can force the lock to be taken in a certain order, which will
> avoid the possibility of deadlock with other such DELETEs.
>

I've changed the wording to address your concerns:

   While doing this will actually increase the total amount of work
performed, it can break the work into chunks that have a more acceptable
impact on other workloads.



>
> About the SELECT example:
> -
>
> That example belongs to UPDATE, I'd say, because that is the main
> operation.
>

I'm iffy on that suggestion. A big part of putting it in SELECT was the
fact that it shows usage of SKIP LOCKED and FOR UPDATE.


>
> The reason you give (avoid excessive locking) is good.
> Perhaps you could mention that updating in batches also avoids
> excessive bload (if you VACUUM between the batches).
>

I went with:

   This technique has the additional benefit that it can reduce the overal
bloat of the updated table if the table can be vacuumed in between batch
updates.


>
> About the UPDATE example:
> -
>
> I think that could go, because it is pretty similar to the previous
> one.  You even use ctid in both examples.
>

It is similar, but the idea here is to aid in discovery. A user might miss
the technique for update if it's only documented in delete, and even if
they did see it there, they might not realize that it works for both UPDATE
and DELETE. We could make reference links from one to the other, but that
seems like extra work for the reader.
From c6179c3cf1395884d4a42b5ad983542a3fc4887c Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Tue, 31 Oct 2023 03:52:41 -0400
Subject: [PATCH v2] Currently we do not show any examples of using ctid
 anywhere, nor do we address the often-requested but problematic use case of
 having a LIMIT clause on UPDATE and DELETE statements. These examples are a
 subtle way of addressing both those concerns.

---
 doc/src/sgml/ref/delete.sgml | 29 +
 doc/src/sgml/ref/select.sgml | 24 
 doc/src/sgml/ref/update.sgml | 23 +++
 3 files changed, 76 insertions(+)

diff --git a/doc/src/sgml/ref/delete.sgml b/doc/src/sgml/ref/delete.sgml
index 1b81b4e7d7..4e08c6c85e 100644
--- a/doc/src/sgml/ref/delete.sgml
+++ b/doc/src/sgml/ref/delete.sgml
@@ -234,6 +234,35 @@ DELETE FROM films
In some cases the join style is easier to write or faster to
execute than the sub-select style.
   
+  
+   In situations where a single operation would consume too many resources,
+   either causing the operation to fail or negatively impacting other workloads,
+   it may be desirable to break up a large DELETE into
+   multiple separate commands. While doing this will actually increase the
+   total amount of work performed, it can break the work into chunks that have
+   a more acceptable impact on other workloads.  The
+   SQL standard does
+   not define a LIMIT clause for DELETE
+   operations, but it is possible get the equivalent functionality through the
+   USING clause to a
+   Common Table Expression which identifies
+   a subset of rows to be deleted, locks those rows, and returns their system
+   column ctid values:
+
+WITH delete_batch AS (
+  SELECT l.ctid
+  FROM user_logs AS l
+  WHERE l.status = 'archived'
+  ORDER BY l.creation_date
+  LIMIT 1
+  FOR UPDATE
+)
+DELETE FROM user_logs AS ul
+USING delete_branch AS del
+WHERE ul.ctid = del.ctid;
+
+  This allows for flexible search criteria within the CTE and an efficient self-join.
+  
  
 
  
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 42d78913cf..10e10ea249 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -1679,6 +1679,30 @@ SELECT * FROM (SELECT * FROM mytable FOR UPDATE) ss WHERE col1 = 5;
 condition is not textually within the sub-query.

 
+   
+In cases where a DML operation involving many rows
+must be performed, and that table experiences numerous other simultaneous
+DML operations, a FOR UPDATE clause
+used in conjunction with SKIP LOCKED can be usefu

Re: SQL:2011 application time

[Corey Huinker]

>
> The PERIOD patch is not finished and includes some deliberately-failing
> tests. I did make some progress here finishing ALTER TABLE ADD PERIOD.
>

If it's ok with you, I need PERIODs for System Versioning, and planned on
developing a highly similar version, albeit closer to the standard. It
shouldn't interfere with your work as you're heavily leveraging range
types.

Re: Statistics Import and Export

[Corey Huinker]

>
>
> Thanks. I think this may be used with postgres_fdw to import
> statistics directly from the foreigns server, whenever possible,
> rather than fetching the rows and building it locally. If it's known
> that the stats on foreign and local servers match for a foreign table,
> we will be one step closer to accurately estimating the cost of a
> foreign plan locally rather than through EXPLAIN.
>
>
Yeah, that use makes sense as well, and if so then postgres_fdw would
likely need to be aware of the appropriate query for several versions back
- they change, not by much, but they do change. So now we'd have each query
text in three places: a system view, postgres_fdw, and the bin/scripts
pre-upgrade program. So I probably should consider the best way to share
those in the codebase.

Re: Document efficient self-joins / UPDATE LIMIT techniques.

[Corey Huinker]

On Wed, Jun 28, 2023 at 2:20 PM Corey Huinker 
wrote:

> This patch adds a few examples to demonstrate the following:
>

Bumping so CF app can see thread.

>

Statistics Import and Export

[Corey Huinker]

pg_stats_export is a view that aggregates pg_statistic data by relation
oid and stores all of the column statistical data in a system-indepdent
(i.e.
no oids, collation information removed, all MCV values rendered as text)
jsonb format, along with the relation's relname, reltuples, and relpages
from pg_class, as well as the schemaname from pg_namespace.

pg_import_rel_stats is a function which takes a relation oid,
server_version_num, num_tuples, num_pages, and a column_stats jsonb in
a format matching that of pg_stats_export, and applies that data to
the specified pg_class and pg_statistics rows for the relation
specified.

The most common use-case for such a function is in upgrades and
dump/restore, wherein the upgrade process would capture the output of
pg_stats_export into a regular table, perform the upgrade, and then
join that data to the existing pg_class rows, updating statistics to be
a close approximation of what they were just prior to the upgrade. The
hope is that these statistics are better than the early stages of
--analyze-in-stages and can be applied faster, thus reducing system
downtime.

The values applied to pg_class are done inline, which is to say
non-transactionally. The values applied to pg_statitics are applied
transactionally, as if an ANALYZE operation was reading from a
cheat-sheet.

This function and view will need to be followed up with corresponding
ones for pg_stastitic_ext and pg_stastitic_ext_data, and while we would
likely never backport the import functions, we can have user programs
do the same work as the export views such that statistics can be brought
forward from versions as far back as there is jsonb to store it.

While the primary purpose of the import function(s) are to reduce downtime
during an upgrade, it is not hard to see that they could also be used to
facilitate tuning and development operations, asking questions like "how
might
this query plan change if this table has 1000x rows in it?", without
actually
putting those rows into the table.
From 834a415f594b3662716c9728a2ab46e425e80e82 Mon Sep 17 00:00:00 2001
From: coreyhuinker 
Date: Thu, 31 Aug 2023 01:30:57 -0400
Subject: [PATCH v1] Introduce the system view pg_stats_export and the function
 pg_import_rel_stats.

pg_stats_export is a view that aggregates pg_statistic data by relation
oid and stores all of the column statistical data in a system-indepdent (i.e.
no oids) jsonb format, along with the relation's relname, reltuples, and
relpages from pg_class, as well as the schemaname from pg_namespace.

pg_import_rel_stats is a function which takes a relation oid,
server_version_num, num_tuples, num_pages, and a column_stats jsonb in
a format matching that of pg_stats_export, and applies that data to
the specified pg_class and pg_statistics rows for the relation
specified.

The most common use-case for such a function is in upgrades and
dump/restore, wherein the upgrade process would capture the output of
pg_stats_export into a regular table, perform the upgrade, and then
join that data to the existing pg_class rows, updating statistics to be
a close approximation of what they were just prior to the upgrade. The
hope is that these statistics are better than the early stages of
--analyze-in-stages and can be applied faster, thus reducing system
downtime.

The values applied to pg_class are done inline, which is to say
non-transactionally. The values applied to pg_statitics are applied
transactionally, as if an ANALYZE operation was reading from a
cheat-sheet.

The statistics applied are no more durable than any other, and will
likely be overwritten by the next autovacuum analyze.
---
 src/include/catalog/pg_proc.dat  |   5 +
 src/include/commands/vacuum.h|   3 +
 src/backend/catalog/system_views.sql |  92 +
 src/backend/commands/analyze.c   | 581 +++
 src/test/regress/expected/rules.out  |  47 +++
 src/test/regress/expected/vacuum.out |  95 +
 src/test/regress/sql/vacuum.sql  |  83 
 doc/src/sgml/func.sgml   |  44 +-
 doc/src/sgml/system-views.sgml   |   5 +
 9 files changed, 954 insertions(+), 1 deletion(-)

diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9805bc6118..48c662c93c 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5636,6 +5636,11 @@
   proname => 'pg_stat_get_db_checksum_last_failure', provolatile => 's',
   proparallel => 'r', prorettype => 'timestamptz', proargtypes => 'oid',
   prosrc => 'pg_stat_get_db_checksum_last_failure' },
+{ oid => '3813',
+  descr => 'statistics: import to relation',
+  proname => 'pg_import_rel_stats', provolatile => 'v', proisstrict => 'f',
+  proparallel => 'u', prorettype => 'bool', proargtypes => 'oid int4 float4 int4 jsonb',
+  prosrc => 'pg_import_rel_stats' },
 { oid => '3074', descr => 'statistics: last reset for a database',
   proname => 'pg_stat_get_db_stat_reset_time', provolatile => 's',
   proparallel => 'r', 

Document efficient self-joins / UPDATE LIMIT techniques.

[Corey Huinker]

This patch adds a few examples to demonstrate the following:

* The existence of the ctid column on every table
* The utility of ctds in self joins
* A practical usage of SKIP LOCKED

The reasoning for this is a bit long, but if you're interested, keep
reading.

In the past, there has been a desire to see a LIMIT clause of some sort on
UPDATE and DELETE statements. The reason for this usually stems from having
a large archive or backfill operation that if done in one single
transaction would overwhelm normal operations, either by the transaction
failing outright, locking too many rows, flooding the WAL causing replica
lag, or starving other processes of limited I/O.

The reasons for not adding a LIMIT clause are pretty straightforward: it
isn't in the SQL Standard, and UPDATE/DELETE operations are unordered
operations, so updating 1000 rows randomly isn't a great idea. The people
wanting the LIMIT clause were undeterred by this, because they know that
they intend to keep issuing updates until they run out of rows to update.

Given these limitations, I would write something like this:

WITH doomed AS (
SELECT t.id
FROM my_table AS t
WHERE t.expiration_date < :'some_archive_date'
FOR UPDATE SKIP LOCKED
LIMIT 1000 )
DELETE FROM my_table
WHERE id IN (SELECT id FROM doomed );

This wouldn't interfere with any other updates, so I felt good about it
running when the system was not-too-busy. I'd then write a script to run
that in a loop, with sleeps to allow the replicas a chance to catch their
breath. Then, when the rowcount finally dipped below 1000, I'd issue the
final

DELETE FROM my_table WHERE expiration_date < :'some_archive_date';

And this was ok, because at that point I have good reason to believe that
there are at most 1000 rows lingering out there, so waiting on locks for
those was no big deal.

But a query like this involves one scan along one index (or worse, a seq
scan) followed by another scan, either index or seq. Either way, we're
taking up a lot of cache with rows we don't even care about.

Then in v12, the query planner got hip to bitmap tidscans, allowing for
this optimization:

WITH doomed AS (
SELECT t.ctid AS tid
FROM my_table AS t
WHERE t.expiration_date < :'some_archive_date'
FOR UPDATE SKIP LOCKED
LIMIT 1000 )
DELETE FROM my_table
USING doomed WHERE my_table.ctid = doomed.tid;

And this works pretty well, especially if you set up a partial index to
meet the quals in the CTE. But we don't document this anywhere, and until
UPDATE and DELETE get a LIMIT clause, we probably should document this
workaround.
From 209fd8abe50603e85ca0cc07aecd72b87889e757 Mon Sep 17 00:00:00 2001
From: coreyhuinker 
Date: Tue, 13 Jun 2023 13:00:40 -0400
Subject: [PATCH v1] Add examples that highlight the usage of the system column
 ctid in self-joins.

Currently we do not show any examples of using ctid anywhere, nor do we
address the often-requested but problematic use case of having a LIMIT
clause on UPDATE and DELETE statements. These examples are a subtle way
of addressing both those concerns.
---
 doc/src/sgml/ref/delete.sgml | 24 
 doc/src/sgml/ref/select.sgml | 21 +
 doc/src/sgml/ref/update.sgml | 23 +++
 3 files changed, 68 insertions(+)

diff --git a/doc/src/sgml/ref/delete.sgml b/doc/src/sgml/ref/delete.sgml
index 1b81b4e7d7..cca9138843 100644
--- a/doc/src/sgml/ref/delete.sgml
+++ b/doc/src/sgml/ref/delete.sgml
@@ -234,6 +234,30 @@ DELETE FROM films
In some cases the join style is easier to write or faster to
execute than the sub-select style.
   
+  
+   In situations where a single operation would consume too many resources, it
+   may be desirable to break up a large DELETE into multiple
+   separate commands. The SQL
+   standard does not define a LIMIT clause for
+   DELETE operations, but it is possible get the equivalent
+   functionality through the USING clause to a
+   Common Table Expression which identifies
+   a subset of rows to be deleted, locks those rows, and returns their system
+   column ctid values:
+
+WITH delete_batch AS (
+  SELECT l.ctid
+  FROM user_logs AS l
+  WHERE l.status = 'archived'
+  ORDER BY l.creation_date
+  LIMIT 1
+  FOR UPDATE )
+DELETE FROM user_logs AS ul
+USING delete_branch AS del
+WHERE ul.ctid = del.ctid
+
+  This allows for flexible search criteria within the CTE and an efficient self-join.
+  
  
 
  
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 0ee0cc7e64..9d7c3d5c41 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -1676,6 +1676,27 @@ SELECT * FROM (SELECT * FROM mytable FOR UPDATE) ss WHERE col1 = 5;
 condition is not textually within the sub-query.

 
+   
+In cases where a DML operation involving many rows
+must be performed, and that table experiences numerous other simultaneous
+DML operations, a FOR UPDATE clause
+used in conjunction with SKIP 

Re: Should heapam_estimate_rel_size consider fillfactor?

[Corey Huinker]

>
> So maybe we should make table_block_relation_estimate_size smarter to
> also consider the fillfactor in the "no statistics" branch, per the
> attached patch.
>

I like this a lot. The reasoning is obvious, the fix is simple,it doesn't
upset any make-check-world tests, and in order to get a performance
regression we'd need a table whose fillfactor has been changed after the
data was loaded but before an analyze happens, and that's a narrow enough
case to accept.

My only nitpick is to swap

(usable_bytes_per_page * fillfactor / 100) / tuple_width

with

(usable_bytes_per_page * fillfactor) / (tuple_width * 100)


as this will eliminate the extra remainder truncation, and it also gets the
arguments "in order" algebraically.

Re: Adding SHOW CREATE TABLE

[Corey Huinker]

>
> I think the ONLY place we should have this is in server side functions.
> More than ten years ago I did some work in this area (see below), but it's
> one of those things that have been on my ever growing personal TODO list
>
> See 
>  and
> 
> 
>
>
>
> Some additional backstory, as this has come up before...

A direction discussion of a previous SHOW CREATE effort:
https://www.postgresql.org/message-id/20190705163203.gd24...@fetter.org

Other databases' implementations of SHOW CREATE came up in this discussion
as well:
https://www.postgresql.org/message-id/CADkLM=fxfsrhaskk_by_a4uomj1te5mfggd_rwwqfv8wp68...@mail.gmail.com

Clearly, there is customer demand for something like this, and has been for
a long time.

Re: Large files for relations

[Corey Huinker]

On Wed, May 3, 2023 at 1:37 AM Thomas Munro  wrote:

> On Wed, May 3, 2023 at 5:21 PM Thomas Munro 
> wrote:
> > rsync --link-dest
>
> I wonder if rsync will grow a mode that can use copy_file_range() to
> share blocks with a reference file (= previous backup).  Something
> like --copy-range-dest.  That'd work for large-file relations
> (assuming a file system that has block sharing, like XFS and ZFS).
> You wouldn't get the "mtime is enough, I don't even need to read the
> bytes" optimisation, which I assume makes all database hackers feel a
> bit queasy anyway, but you'd get the space savings via the usual
> rolling checksum or a cheaper version that only looks for strong
> checksum matches at the same offset, or whatever other tricks rsync
> might have up its sleeve.
>

I understand the need to reduce open file handles, despite the
possibilities enabled by using large numbers of small file sizes.
Snowflake, for instance, sees everything in 1MB chunks, which makes
massively parallel sequential scans (Snowflake's _only_ query plan)
possible, though I don't know if they accomplish that via separate files,
or via segments within a large file.

I am curious whether a move like this to create a generational change in
file file format shouldn't be more ambitious, perhaps altering the block
format to insert a block format version number, whether that be at every
block, or every megabyte, or some other interval, and whether we store it
in-file or in a separate file to accompany the first non-segmented. Having
such versioning information would allow blocks of different formats to
co-exist in the same table, which could be critical to future changes such
as 64 bit XIDs, etc.

Re: Note new NULLS NOT DISTINCT on unique index tutorial page

[Corey Huinker]

On Wed, Apr 12, 2023 at 10:40 AM David Gilman 
wrote:

> The SQL Language part of the docs has a brief page on unique indexes.
> It doesn't mention the new NULLS NOT DISTINCT functionality on unique
> indexes and this is a good place to mention it, as it already warns
> the user about the old/default behavior.
>
>
I'm ok with the wording as-is, but perhaps we can phrase it as "distinct"
vs "not equal", thus leaning into the syntax a bit:

By default, null values in a unique column are considered distinct,
allowing multiple nulls in the column.


or maybe

By default, null values in a unique column are considered
DISTINCT, allowing multiple nulls in the column.

Re: Add SHELL_EXIT_CODE to psql

[Corey Huinker]

On Fri, Mar 24, 2023 at 5:21 PM Corey Huinker 
wrote:

>
>
> On Tue, Mar 21, 2023 at 3:33 PM Corey Huinker 
> wrote:
>
>>
>>> As you had it, any nonzero result would prevent backtick substitution.
>>> I'm not really sure how much thought went into the existing behavior,
>>> but I am pretty sure that it's not part of this patch's charter to
>>> change that.
>>>
>>> regards, tom lane
>>>
>>
>>  The changes all make sense, thanks!
>>
>
> This is a follow up patch to apply the committed pattern to the various
> piped output commands.
>
> I spotted this oversight in the
> https://www.postgresql.org/message-id/CADkLM=dMG6AAWfeKvGnKOzz1O7ZNctFR1BzAA3K7-+XQxff=4...@mail.gmail.com
> thread and, whether or not that feature gets in, we should probably apply
> it to output pipes as well.
>

Following up here. This addendum patch clearly isn't as important as
anything currently trying to make it in before the feature freeze, but I
think the lack of setting the SHELL_ERROR and SHELL_EXIT_CODE vars on piped
commands would be viewed as a bug, so I'm hoping somebody can take a look
at it.

Re: Thoughts on using Text::Template for our autogenerated code?

[Corey Huinker]

>
> Yeah, it's somewhat hard to believe that the cost/benefit ratio would be
> attractive.  But maybe you could mock up some examples of what the input
> could look like, and get people on board (or not) before writing any
> code.
>
>
tl;dr - I tried a few things, nothing that persuades myself let alone the
community, but perhaps some ideas for the future.

I borrowed Bertrand's ongoing work for waiteventnames.* because that is
what got me thinking about this in the first place. I considered a few
different templating libraries:

There is no perl implementation of the golang template library (example of
that here: https://blog.gopheracademy.com/advent-2017/using-go-templates/ )
that I could find.

Text::Template does not support loops, and as such it is no better than
here-docs.

Template Toolkit seems to do what we need, but it has a kitchen sink of
dependencies that make it an unattractive option, so I didn't even attempt
it.

HTML::Template has looping and if/then/else constructs, and it is a single
standalone library. It also does a "separation of concerns" wherein you
pass in parameter names and values, and some parameters can be for loops,
which means you pass an arrayref of hashrefs that the template engine loops
over. That's where the advantages stop, however. It is fairly verbose, and
because it is HTML-centric it isn't very good about controlling whitespace,
which leads to piling template directives onto the same line in order to
avoid spurious newlines. As such I cannot recommend it.

My ideal template library would have text something like this:

[% loop events %]
[% $enum_value %]
[% if __first__ +%] = [%+ $inital_value %][% endif %]
[% if ! __last__ %],[% endif +%]
[% end loop %]
[% loop xml_blocks indent: relative,spaces,4 %]



  [%element_body%]/>



[% end loop %]


[%+ means "leading whitespace matters", +%] means "trailing whitespace
matters"
That pseudocode is a mix of ASP, HTML::Template. The special variables
__first__ and __last__ refer to which iteration of the loop we are on. You
would pass it a data structure like this:

{events: [ { enum_value: "abc", initial_value: "def"}, ... { enum_value:
"wuv", initial_value: "xyz" } ],
 xml_block: [ {attrib_val: "one", element_body: "two"} ]
 }


I did one initial pass with just converting printf statements to here-docs,
and the results were pretty unsatisfying. It wasn't really possible to
"see" the output files take shape.

My next attempt was to take the "separation of concerns" code from the
HTML::Template version, constructing the nested data structure of resolved
output values, and then iterating over that once per output file. This
resulted in something cleaner, partly because we're only writing one file
type at a time, partly because the interpolated variables have names much
closer to their output meaning.

In doing this, it occurred to me that a lot of this effort is in getting
the code to conform to our own style guide, at the cost of the generator
code being less readable. What if we wrote the generator and formatted the
code in a way that made sense for the generator, and then pgindented it.
That's not the workflow right now, but perhaps it could be.

Conclusions:
- There is no "good enough" template engine that doesn't require big
changes in dependencies.
- pgindent will not save you from a run-on sentence, like putting all of
a typdef enum values on one line
- There is some clarity value in either separating input processing from
the output processing, or making the input align more closely with the
outputs
- Fiddling with indentation and spacing detracts from legibility no matter
what method is used.
- here docs are basically ok but they necessarily confuse output
indentation with code indentation. it is possible to de-indent them them
with <<~ but that's a 5.25+ feature.
- Any of these principles can be applied at any time, with no overhaul
required.


"sorted-" is the slightly modified version of Bertrand's code.
"eof-as-is-" is a direct conversion of the original but using here-docs.
"heredoc-fone-file-at-a-time-" first generates an output-friendly data
structure
"needs-pgindent-" is what is possible if we format for our own readability
and make pgindent fix the output, though it was not a perfect output match


sorted-generate-waiteventnames.pl
Description: Perl program


eof-as-is-generate-waiteventnames.pl
Description: Perl program


heredoc-one-file-at-a-time-generate-waiteventnames.pl
Description: Perl program


needs-pgindent-generate-waiteventnames.pl
Description: Perl program

Re: Thoughts on using Text::Template for our autogenerated code?

[Corey Huinker]

>
> I don't think that's remotely a starter. Asking people to install an old
> and possibly buggy version of a perl module is not something we should do.
>
> I think the barrier for this is pretty high. I try to keep module
> requirements for the buildfarm client pretty minimal, and this could affect
> a much larger group of people.
>
Those are good reasons.

For those who already responded, are your concerns limited to the
dependency issue? Would you have concerns about a templating library that
was developed by us and included in-tree? I'm not suggesting suggesting we
write one at this time, at least not until after we make a here-doc-ing
pass first, but I want to understand the concerns before embarking on any
refactoring.

Re: Thoughts on using Text::Template for our autogenerated code?

[Corey Huinker]

>
> I think many of those could just be replaced by multi-line strings and/or
> here
> documents to get most of the win.
>

I agree that a pretty good chunk of it can be done with here-docs, but
template files do have attractive features (separation of concerns, syntax
highlighting, etc) that made it worth asking.

Thoughts on using Text::Template for our autogenerated code?

[Corey Huinker]

Is there a barrier to us using non-core perl modules, in this case
Text::Template?

I think it would be a tremendous improvement in readability and
maintainability over our current series of print statements, some
multiline, some not.

The module itself works like this https://www.perlmonks.org/?node_id=33296

Some other digging around shows that the module has been around since 1996
(Perl5 was 1994) and hasn't had a feature update (or any update for that
matter) since 2003. So it should meet our baseline 5.14 requirement, which
came out in 2011.

I'm happy to proceed with a proof-of-concept so that people can see the
costs/benefits, but wanted to first make sure it wasn't a total non-starter.

Re: Autogenerate some wait events code and documentation

[Corey Huinker]

On Wed, Mar 29, 2023 at 8:51 AM Drouvot, Bertrand <
bertranddrouvot...@gmail.com> wrote:

> Hi,
>
> On 3/29/23 11:44 AM, Drouvot, Bertrand wrote:
>
> >
> > Looking forward to your feedback,
>
> Just realized that more polishing was needed.
>
> Done in V2 attached.
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com


I think this is good work, but I can't help thinking it would be easier to
understand and maintain if we used a template engine like Text::Template,
and filled out the template with the variant bits. I'll ask that question
in another thread for higher visibility.

Re: Using Ephemeral Named Relation like a temporary table

[Corey Huinker]

On Wed, Mar 29, 2023 at 12:54 AM Yugo NAGATA  wrote:

> Hello,
>
>
> Temporary tables are often used to store transient data in
> batch processing and the contents can be accessed multiple
> times. However, frequent use of temporary tables has a problem
> that the system catalog tends to bloat. I know there has been
> several proposals to attack this problem, but I would like to
> propose a new one.
>
> The idea is to use Ephemeral Named Relation (ENR) like a
> temporary table. ENR information is not stored into the system
> catalog, but in QueryEnvironment, so it never bloat the system
> catalog.
>
> Although we cannot perform insert, update or delete on ENR,
> I wonder it could be beneficial if we need to reference to a
> result of a query multiple times in a batch processing.
>
> The attached is a concept patch. This adds a new syntax
> "OPEN cursor INTO TABLE tablename" to pl/pgSQL, that stores
> a result of the cursor query into a ENR with specified name.
> However, this is a tentative interface to demonstrate the
> concept of feature.
>
> Here is an example;
>
> postgres=# \sf fnc
> CREATE OR REPLACE FUNCTION public.fnc()
>  RETURNS TABLE(sum1 integer, avg1 integer, sum2 integer, avg2 integer)
>  LANGUAGE plpgsql
> AS $function$
> DECLARE
>  sum1 integer;
>  sum2 integer;
>  avg1 integer;
>  avg2 integer;
>  curs CURSOR FOR SELECT aid, bid, abalance FROM pgbench_accounts
>WHERE abalance BETWEEN 100 AND 200;
> BEGIN
>  OPEN curs INTO TABLE tmp_accounts;
>  SELECT count(abalance) , avg(abalance) INTO sum1, avg1
>FROM tmp_accounts;
>  SELECT count(bbalance), avg(bbalance) INTO sum2, avg2
>FROM tmp_accounts a, pgbench_branches b WHERE a.bid = b.bid;
>  RETURN QUERY SELECT sum1,avg1,sum2,avg2;
> END;
> $function$
>
> postgres=# select fnc();
> fnc
> 
>  (541,151,541,3937)
> (1 row)
>
> As above, we can use the same query result for multiple
> aggregations, and also join it with other tables.
>
> What do you think of using ENR for this way?
>
> Regards,
> Yugo Nagata
>
> --
> Yugo NAGATA 
>

This looks like a slightly more flexible version of the Oracle pl/sql table
type.

For those not familiar, PL/SQL can have record types, and in-memory
collections of records types, and you can either build up multiple records
in a collection manually, or you can bulk-collect them from a query. Then,
you can later reference that collection in a regular SQL query with FROM
TABLE(collection_name). It's a neat system for certain types of workloads.

example link, I'm sure there's better out there:
https://oracle-base.com/articles/12c/using-the-table-operator-with-locally-defined-types-in-plsql-12cr1

My first take is there are likely customers out there that will want this.
However, those customers will want to manually add/delete rows from the
ENR, so we'll want a way to do that.

I haven't looked at ENRs in a while, when would the memory from that ENR
get freed?

Re: CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions

[Corey Huinker]

On Tue, Mar 28, 2023 at 3:25 PM Isaac Morland 
wrote:

> On Mon, 19 Dec 2022 at 17:57, Corey Huinker 
> wrote:
>
>>
>> Attached is my work in progress to implement the changes to the CAST()
>> function as proposed by Vik Fearing.
>>
>> CAST(expr AS typename NULL ON ERROR)
>> will use error-safe functions to do the cast of expr, and will return
>> NULL if the cast fails.
>>
>> CAST(expr AS typename DEFAULT expr2 ON ERROR)
>> will use error-safe functions to do the cast of expr, and will return
>> expr2 if the cast fails.
>>
>
> Is there any difference between NULL and DEFAULT NULL?
>

What I think you're asking is: is there a difference between these two
statements:

SELECT CAST(my_string AS integer NULL ON ERROR) FROM my_table;


SELECT CAST(my_string AS integer DEFAULT NULL ON ERROR) FROM my_table;


And as I understand it, the answer would be no, there is no practical
difference. The first case is just a convenient shorthand, whereas the
second case tees you up for a potentially complex expression. Before you
ask, I believe the ON ERROR syntax could be made optional. As I implemented
it, both cases create a default expression which then typecast to integer,
and in both cases that expression would be a const-null, so the optimizer
steps would very quickly collapse those steps into a plain old constant.

Re: CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions

[Corey Huinker]

On Tue, Mar 28, 2023 at 2:53 PM Gregory Stark (as CFM) 
wrote:

> On Tue, 3 Jan 2023 at 14:16, Tom Lane  wrote:
> >
> > Vik Fearing  writes:
> >
> > > I don't think we should add that syntax until I do get it through the
> > > committee, just in case they change something.
> >
> > Agreed.  So this is something we won't be able to put into v16;
> > it'll have to wait till there's something solid from the committee.
>
> I guess I'll mark this Rejected in the CF then. Who knows when the SQL
> committee will look at this...
>
> --
> Gregory Stark
> As Commitfest Manager
>

Yes, for now. I'm in touch with the pg-people on the committee and will
resume work when there's something to act upon.

Re: Add SHELL_EXIT_CODE to psql

[Corey Huinker]

On Tue, Mar 21, 2023 at 3:33 PM Corey Huinker 
wrote:

>
>> As you had it, any nonzero result would prevent backtick substitution.
>> I'm not really sure how much thought went into the existing behavior,
>> but I am pretty sure that it's not part of this patch's charter to
>> change that.
>>
>> regards, tom lane
>>
>
>  The changes all make sense, thanks!
>

This is a follow up patch to apply the committed pattern to the various
piped output commands.

I spotted this oversight in the
https://www.postgresql.org/message-id/CADkLM=dMG6AAWfeKvGnKOzz1O7ZNctFR1BzAA3K7-+XQxff=4...@mail.gmail.com
thread and, whether or not that feature gets in, we should probably apply
it to output pipes as well.
From 9e29989372b7f42fcb7eac6dd15769ebe643abb5 Mon Sep 17 00:00:00 2001
From: coreyhuinker 
Date: Fri, 24 Mar 2023 17:20:27 -0400
Subject: [PATCH v1] Have pipes set SHELL_ERROR and SHELL_EXIT_CODE.

---
 src/bin/psql/common.c | 20 +---
 src/bin/psql/copy.c   |  4 
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index f907f5d4e8..3be24250ad 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -103,7 +103,13 @@ setQFout(const char *fname)
 	if (pset.queryFout && pset.queryFout != stdout && pset.queryFout != stderr)
 	{
 		if (pset.queryFoutPipe)
-			pclose(pset.queryFout);
+		{
+			char	buf[32];
+			int		exit_code = pclose(pset.queryFout);
+			snprintf(buf, sizeof(buf), "%d", wait_result_to_exit_code(exit_code));
+			SetVariable(pset.vars, "SHELL_EXIT_CODE", buf);
+			SetVariable(pset.vars, "SHELL_ERROR", (exit_code == 0) ? "false" : "true");
+		}
 		else
 			fclose(pset.queryFout);
 	}
@@ -1652,7 +1658,11 @@ ExecQueryAndProcessResults(const char *query,
 	{
 		if (gfile_is_pipe)
 		{
-			pclose(gfile_fout);
+			char	buf[32];
+			int		exit_code = pclose(gfile_fout);
+			snprintf(buf, sizeof(buf), "%d", wait_result_to_exit_code(exit_code));
+			SetVariable(pset.vars, "SHELL_EXIT_CODE", buf);
+			SetVariable(pset.vars, "SHELL_ERROR", (exit_code == 0) ? "false" : "true");
 			restore_sigpipe_trap();
 		}
 		else
@@ -1870,7 +1880,11 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
 		/* close \g argument file/pipe */
 		if (is_pipe)
 		{
-			pclose(fout);
+			char	buf[32];
+			int		exit_code = pclose(fout);
+			snprintf(buf, sizeof(buf), "%d", wait_result_to_exit_code(exit_code));
+			SetVariable(pset.vars, "SHELL_EXIT_CODE", buf);
+			SetVariable(pset.vars, "SHELL_ERROR", (exit_code == 0) ? "false" : "true");
 			restore_sigpipe_trap();
 		}
 		else
diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c
index 110d2d7269..00e01095fe 100644
--- a/src/bin/psql/copy.c
+++ b/src/bin/psql/copy.c
@@ -375,6 +375,7 @@ do_copy(const char *args)
 	{
 		if (options->program)
 		{
+			char		buf[32];
 			int			pclose_rc = pclose(copystream);
 
 			if (pclose_rc != 0)
@@ -391,6 +392,9 @@ do_copy(const char *args)
 }
 success = false;
 			}
+			snprintf(buf, sizeof(buf), "%d", wait_result_to_exit_code(pclose_rc));
+			SetVariable(pset.vars, "SHELL_EXIT_CODE", buf);
+			SetVariable(pset.vars, "SHELL_ERROR", (pclose_rc == 0) ? "false" : "true");
 			restore_sigpipe_trap();
 		}
 		else
-- 
2.39.2

Re: Make ON_ERROR_STOP stop on shell script failure

[Corey Huinker]

On Fri, Mar 24, 2023 at 2:16 PM Corey Huinker 
wrote:

>
>
> On Fri, Mar 24, 2023 at 11:07 AM Peter Eisentraut <
> peter.eisentr...@enterprisedb.com> wrote:
>
>> On 20.03.23 19:31, Greg Stark wrote:
>> > So I took a look at this patch. The conflict is with 2fe3bdbd691
>> > committed by Peter Eisentraut which added error checks for pipes.
>> > Afaics the behaviour is now for pipe commands returning non-zero to
>> > cause an error*always*  regardless of the setting of ON_ERROR_STOP.
>>
>
> Commit b0d8f2d983cb25d1035fae1cd7de214dd67809b4 adds SHELL_ERROR as a set
> to 'true' whenever a \! or backtick command has a nonzero exit code. So
> it'll need some rebasing to remove the duplicated work.
>
> So it's now possible to do this:
>
> \set result = `some command`
> \if :SHELL_ERROR
>-- maybe test SHELL_EXIT_CODE to see what kind of error
>\echo some command failed
>-- nah, just quit
>\q
> \endif
>
>
>> > I'm not entirely sure that's sensible actually. If you write to a pipe
>> > that ends in grep and it happens to produce no matching rows you may
>> > actually be quite surprised when that causes your script to fail...
>>
>
> I agree that that would be quite surprising, and this feature would be a
> non-starter for them. But if we extended the SHELL_ERROR and
> SHELL_EXIT_CODE patch to handle output pipes (which maybe we should have
> done in the first place), the handling would look like this:
>
> SELECT ... \g grep Frobozz
> \if :SHELL_ERROR
>   SELECT :SHELL_EXIT_CODE = 1 AS grep_found_nothing \gset
>   \if :grep_found_nothing
>   ...not-a-real-error handling...
>   \else
>   ...regular error handling...
>   \endif
> \endif
>
> ...and that would be the solution for people who wanted to do something
> more nuanced than ON_ERROR_STOP.
>
>
Dangit. Replied to Peter's email thinking he had gone off Greg's culling of
the recipients. Re-culled.

Re: Make ON_ERROR_STOP stop on shell script failure

[Corey Huinker]

On Fri, Mar 24, 2023 at 11:07 AM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:

> On 20.03.23 19:31, Greg Stark wrote:
> > So I took a look at this patch. The conflict is with 2fe3bdbd691
> > committed by Peter Eisentraut which added error checks for pipes.
> > Afaics the behaviour is now for pipe commands returning non-zero to
> > cause an error*always*  regardless of the setting of ON_ERROR_STOP.
>

Commit b0d8f2d983cb25d1035fae1cd7de214dd67809b4 adds SHELL_ERROR as a set
to 'true' whenever a \! or backtick command has a nonzero exit code. So
it'll need some rebasing to remove the duplicated work.

So it's now possible to do this:

\set result = `some command`
\if :SHELL_ERROR
   -- maybe test SHELL_EXIT_CODE to see what kind of error
   \echo some command failed
   -- nah, just quit
   \q
\endif


> > I'm not entirely sure that's sensible actually. If you write to a pipe
> > that ends in grep and it happens to produce no matching rows you may
> > actually be quite surprised when that causes your script to fail...
>

I agree that that would be quite surprising, and this feature would be a
non-starter for them. But if we extended the SHELL_ERROR and
SHELL_EXIT_CODE patch to handle output pipes (which maybe we should have
done in the first place), the handling would look like this:

SELECT ... \g grep Frobozz
\if :SHELL_ERROR
  SELECT :SHELL_EXIT_CODE = 1 AS grep_found_nothing \gset
  \if :grep_found_nothing
  ...not-a-real-error handling...
  \else
  ...regular error handling...
  \endif
\endif

...and that would be the solution for people who wanted to do something
more nuanced than ON_ERROR_STOP.

Re: doc: add missing "id" attributes to extension packaging page

[Corey Huinker]

>
> TBH I'm a bit afraid that people will immediately start complaining
> about the failing docs builds after this got applied since it forces
> them to add ids to all varlistenries in a variablelist if they add one,
> which can be perceived as quite a burden (also committers and reviewers
> will have to get used to always watch out for failing docs builds
> because of this).
>

As a person who had to re-rebase a patch because I discovered that id tags
had been added to one of the files I was working on, I can confidently say
"don't worry". It wasn't that big of a deal, I wasn't even following this
thread at the time and I immediately figured out what had happened and what
was expected of me. So it isn't that much of an inconvenience. If there is
a negative consequence to this change, it would be that it might
incentivize patch writers to omit documentation completely at the early
stages. But I don't think that's a problem because people generally see a
lack of documentation as a clue that maybe the patch isn't ready to be
reviewed, and this change would only reinforce that litmus test.

I had suggested we do something like this a few years back (the ids, that
is. the idea that we could check for compliance was beyond my imagination
at the time), and I'm glad to see both finally happening.

While I can foresee people overlooking the docs build, such oversights
won't go long before being caught, and the fix is simple.  Now if we can
just get a threaded version of xlstproc to make the builds faster...

p.s. I'm "Team Paperclip" when it comes to the link hint, but let's get the
feature in first and worry about the right character later.

Re: Add n_tup_newpage_upd to pg_stat table views

[Corey Huinker]

>
>
> * No more dedicated struct to carry around the type of an update.
>
> We just use two boolean arguments to the pgstats function instead. The
> struct didn't seem to be adding much, and it was distracting to track
> the information this way within heap_update().
>

That's probably a good move, especially if we start tallying updates that
use TOAST.

Re: Add SHELL_EXIT_CODE to psql

[Corey Huinker]

>
>
> As you had it, any nonzero result would prevent backtick substitution.
> I'm not really sure how much thought went into the existing behavior,
> but I am pretty sure that it's not part of this patch's charter to
> change that.
>
> regards, tom lane
>

 The changes all make sense, thanks!

Re: Add SHELL_EXIT_CODE to psql

[Corey Huinker]

On Mon, Mar 20, 2023 at 1:01 PM Tom Lane  wrote:

> Corey Huinker  writes:
> > 128+N is implemented.
>
> I think this mostly looks OK, but:
>
> * I still say there is no basis whatever for believing that the result
> of ferror() is an exit code, errno code, or anything else with
> significance beyond zero-or-not.  Feeding it to wait_result_to_exit_code
> as you've done here is not going to do anything but mislead people in
> a platform-dependent way.  Probably should set exit_code to -1 if
> ferror reports trouble.
>

Agreed. Sorry, I read your comment wrong the last time or I would have
already made it so.


>
> * Why do you have wait_result_to_exit_code defaulting to return 0
> if it doesn't recognize the code as either WIFEXITED or WIFSIGNALED?
> That seems pretty misleading; again -1 would be a better idea.
>

That makes sense as well. Given that WIFSIGNALED is currently defined as
the negation of WIFEXITED, whatever default result we have here is
basically a this-should-never-happen. That might be something we want to
catch with an assert, but I'm fine with a -1 for now.
From 1ff2b5ba4b82efca0edf60a9fb1cdf8307471eee Mon Sep 17 00:00:00 2001
From: coreyhuinker 
Date: Fri, 17 Mar 2023 06:43:24 -0400
Subject: [PATCH v11 1/3] Add PG_OS_TARGET environment variable to enable
 OS-specific changes to regression tests.

---
 src/test/regress/pg_regress.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 7b23cc80dc..333aaab139 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -594,6 +594,17 @@ initialize_environment(void)
 #endif
 	}
 
+		/*
+		 * Regression tests that invoke OS commands must occasionally use
+		 * OS-specific syntax (example: NUL vs /dev/null). This env var allows
+		 * those tests to adjust commands accordingly.
+		 */
+#if defined(WIN32)
+		setenv("PG_OS_TARGET", "WIN32", 1);
+#else
+		setenv("PG_OS_TARGET", "OTHER", 1);
+#endif
+
 	/*
 	 * Set translation-related settings to English; otherwise psql will
 	 * produce translated messages and produce diffs.  (XXX If we ever support
-- 
2.39.2

From 3d9c43cfec7f60785c1f1fa1aaa3e1b48224ef98 Mon Sep 17 00:00:00 2001
From: coreyhuinker 
Date: Mon, 20 Mar 2023 16:05:10 -0400
Subject: [PATCH v11 2/3] Add wait_result_to_exit_code().

---
 src/common/wait_error.c | 15 +++
 src/include/port.h  |  1 +
 2 files changed, 16 insertions(+)

diff --git a/src/common/wait_error.c b/src/common/wait_error.c
index 4a3c3c61af..80b94cae51 100644
--- a/src/common/wait_error.c
+++ b/src/common/wait_error.c
@@ -127,3 +127,18 @@ wait_result_is_any_signal(int exit_status, bool include_command_not_found)
 		return true;
 	return false;
 }
+
+/*
+ * Return the POSIX exit code (0 to 255) that corresponds to the argument.
+ * The argument is a return code returned by wait(2) or waitpid(2), which
+ * also applies to pclose(3) and system(3).
+ */
+int
+wait_result_to_exit_code(int exit_status)
+{
+	if (WIFEXITED(exit_status))
+		return WEXITSTATUS(exit_status);
+	if (WIFSIGNALED(exit_status))
+		return 128 + WTERMSIG(exit_status);
+	return -1;
+}
diff --git a/src/include/port.h b/src/include/port.h
index e66193bed9..1b119a3c56 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -495,6 +495,7 @@ extern char *escape_single_quotes_ascii(const char *src);
 extern char *wait_result_to_str(int exitstatus);
 extern bool wait_result_is_signal(int exit_status, int signum);
 extern bool wait_result_is_any_signal(int exit_status, bool include_command_not_found);
+extern int wait_result_to_exit_code(int exit_status);
 
 /*
  * Interfaces that we assume all Unix system have.  We retain individual macros
-- 
2.39.2

From bf65f36fd57977acaac57972425d1717a205cb72 Mon Sep 17 00:00:00 2001
From: coreyhuinker 
Date: Mon, 20 Mar 2023 16:05:23 -0400
Subject: [PATCH v11 3/3] Add psql variables SHELL_ERROR and SHELL_EXIT_CODE
 which report the success or failure of the most recent psql OS-command
 executed via the \! command or `backticks`. These variables are roughly
 analogous to ERROR and SQLSTATE, but for OS level operations instead of SQL
 commands.

---
 doc/src/sgml/ref/psql-ref.sgml | 21 ++
 src/bin/psql/command.c | 15 +
 src/bin/psql/help.c|  4 
 src/bin/psql/psqlscanslash.l   | 33 +
 src/test/regress/expected/psql.out | 34 ++
 src/test/regress/sql/psql.sql  | 30 ++
 6 files changed, 133 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 7b8ae9fac3..e6e307fd18 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4267,6 +4267,27 @@ bar
 
   
 
+  
+   SHELL_ERROR
+   
+
+

Re: Add SHELL_EXIT_CODE to psql

[Corey Huinker]

On Fri, Mar 10, 2023 at 3:49 PM Tom Lane  wrote:

> I'm okay with adopting bash's rule, but then it should actually match
> bash --- signal N is reported as 128+N, not -N.
>

128+N is implemented.

Nonzero pclose errors of any kind will now overwrite an existing exit_code.

I didn't write a formal test for the signals, but instead tested it
manually:

[310444:16:54:32 EDT] corey=# \! sleep 1000
-- in another window, i found the pid of the sleep command and did a kill
-9 on it
[310444:16:55:15 EDT] corey=# \echo :SHELL_EXIT_CODE
137


137 = 128+9, so that checks out.

The OS-specific regression test has been modified. On Windows systems it
attempts to execute "CON", and on other systems it attempts to execute "/".
These are marginally better than "nosuchcommand" in that they should always
exist on their host OS and could never be a legit executable. I have no
opinion whether the test should live on past the development phase, but if
it does not then the 0001 patch is not needed.
From a5dddedcc7bf654b4b65c14ff7b89b9d83bb5c27 Mon Sep 17 00:00:00 2001
From: coreyhuinker 
Date: Fri, 17 Mar 2023 06:43:24 -0400
Subject: [PATCH v9 1/3] Add PG_OS_TARGET environment variable to enable
 OS-specific changes to regression tests.

---
 src/test/regress/pg_regress.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 7b23cc80dc..333aaab139 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -594,6 +594,17 @@ initialize_environment(void)
 #endif
 	}
 
+		/*
+		 * Regression tests that invoke OS commands must occasionally use
+		 * OS-specific syntax (example: NUL vs /dev/null). This env var allows
+		 * those tests to adjust commands accordingly.
+		 */
+#if defined(WIN32)
+		setenv("PG_OS_TARGET", "WIN32", 1);
+#else
+		setenv("PG_OS_TARGET", "OTHER", 1);
+#endif
+
 	/*
 	 * Set translation-related settings to English; otherwise psql will
 	 * produce translated messages and produce diffs.  (XXX If we ever support
-- 
2.39.2

From fe578661eef7dbe33aec8f4eaa6dca80a1304c9b Mon Sep 17 00:00:00 2001
From: coreyhuinker 
Date: Fri, 17 Mar 2023 06:44:43 -0400
Subject: [PATCH v9 2/3] Add wait_result_to_exit_code().

---
 src/common/wait_error.c | 15 +++
 src/include/port.h  |  1 +
 2 files changed, 16 insertions(+)

diff --git a/src/common/wait_error.c b/src/common/wait_error.c
index 4a3c3c61af..8e1c0e3fc8 100644
--- a/src/common/wait_error.c
+++ b/src/common/wait_error.c
@@ -127,3 +127,18 @@ wait_result_is_any_signal(int exit_status, bool include_command_not_found)
 		return true;
 	return false;
 }
+
+/*
+ * Return the POSIX exit code (0 to 255) that corresponds to the argument.
+ * The argument is a return code returned by wait(2) or waitpid(2), which
+ * also applies to pclose(3) and system(3).
+ */
+int
+wait_result_to_exit_code(int exit_status)
+{
+	if (WIFEXITED(exit_status))
+		return WEXITSTATUS(exit_status);
+	if (WIFSIGNALED(exit_status))
+		return 128 + WTERMSIG(exit_status);
+	return 0;
+}
diff --git a/src/include/port.h b/src/include/port.h
index e66193bed9..1b119a3c56 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -495,6 +495,7 @@ extern char *escape_single_quotes_ascii(const char *src);
 extern char *wait_result_to_str(int exitstatus);
 extern bool wait_result_is_signal(int exit_status, int signum);
 extern bool wait_result_is_any_signal(int exit_status, bool include_command_not_found);
+extern int wait_result_to_exit_code(int exit_status);
 
 /*
  * Interfaces that we assume all Unix system have.  We retain individual macros
-- 
2.39.2

From 251863ddcbc4eecf4fa15e332724acbd3c652478 Mon Sep 17 00:00:00 2001
From: coreyhuinker 
Date: Fri, 17 Mar 2023 16:46:57 -0400
Subject: [PATCH v9 3/3] Add psql variables SHELL_ERROR and SHELL_EXIT_CODE
 which report the success or failure of the most recent psql OS-command
 executed via the \! command or `backticks`. These variables are roughly
 analogous to ERROR and SQLSTATE, but for OS level operations instead of SQL
 commands.

---
 doc/src/sgml/ref/psql-ref.sgml | 21 ++
 src/bin/psql/command.c | 15 +
 src/bin/psql/help.c|  4 
 src/bin/psql/psqlscanslash.l   | 35 +-
 src/test/regress/expected/psql.out | 34 +
 src/test/regress/sql/psql.sql  | 30 +
 6 files changed, 134 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 7b8ae9fac3..e6e307fd18 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4267,6 +4267,27 @@ bar
 
   
 
+  
+   SHELL_ERROR
+   
+
+ true if the last shell command failed, false if
+ it succeeded. This applies to shell commands invoked via either \!
+ or `. See also SHELL_EXIT_CODE.
+
+   
+  
+
+  
+  

Re: Add SHELL_EXIT_CODE to psql

[Corey Huinker]

On Thu, Mar 2, 2023 at 1:35 PM Tom Lane  wrote:

> Corey Huinker  writes:
> > [ v9-0003-Add-psql-variables-SHELL_ERROR-and-SHELL_EXIT_COD.patch ]
>
> I took a brief look through this.  I'm on board with the general
> concept, but I think you've spent too much time on an ultimately
> futile attempt to get a committable test case, and not enough time
>

I didn't want to give the impression that I was avoiding/dismissing the
test case, and at some point I got curious how far we could push pg_regress.


> on making the behavior correct/usable.  In particular, it bothers
> me that there doesn't appear to be any way to distinguish whether
> a command failed on nonzero exit code versus a signal.  Also,
>

That's an intriguing distinction, and one I hadn't considered, mostly
because I assumed that any command with a set of exit codes rich enough to
warrant inspection would have a separate exit code for i-was-terminated.

It would certainly be possible to have two independent booleans,
SHELL_ERROR and SHELL_SIGNAL.


> I see that you're willing to report whatever ferror returns
> (something quite unspecified in relevant standards, beyond
> zero/nonzero) as an equally-non-distinguishable integer.
>

I had imagined users of this feature falling into two camps:

1. Users writing scripts for their specific environment, with the ability
to know what exit codes are possible and different desired courses of
action based on those codes.
2. Users in no specific environment, content with SHELL_ERROR boolean, who
are probably just going to test for failure, and if so either \set a
default or \echo a message and \quit.



>
> I'm tempted to suggest that we ought to be returning a string,
> along the lines of "OK" or "exit code N" or "signal N" or
> "I/O error".  This though brings up the question of exactly
> what you expect scripts to use the variable for.  Maybe such
> a definition would be unhelpful, but if so why?  Maybe we
> should content ourselves with adding the SHELL_ERROR boolean, and
> leave the details to whatever we print in the error message?
>

Having a curated list of responses like "OK" and "exit code N" is also
intriguing, but could be hard to unpack given psql's limited string
manipulation capabilities.

I think the SHELL_ERROR boolean solves most cases, but it was suggested in
https://www.postgresql.org/message-id/20221102115801.gg16...@telsasoft.com
that users might want more detail than that, even if that detail is
unspecified and highly system dependent.


>
> As for the test case, I'm unimpressed with it because it's so
> weak as to be nearly useless; plus it seems like a potential
> security issue (what if "nosuchcommand" exists?).  It's fine
> to have it during development, I guess, but I'm inclined to
> leave it out of the eventual commit.
>
>
I have no objection to leaving it out. I think it proves that writing
os-specific pg_regress tests is hard, and little else.

Re: verbose mode for pg_input_error_message?

[Corey Huinker]

On Thu, Feb 23, 2023 at 4:47 PM Nathan Bossart 
wrote:

> On Thu, Feb 23, 2023 at 11:30:38AM -0800, Nathan Bossart wrote:
> > Will post a new version shortly.
>
> As promised...
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com


Looks good to me, passes make check-world. Thanks for slogging through this.

Re: Disable vacuuming to provide data history

[Corey Huinker]

On Thu, Feb 23, 2023 at 6:04 AM  wrote:

> Hey,
>
> It depnends on scenario, but there is many use cases that hack data
> change from somebody with admin privileges could be disaster.
> That is the place where data history could come with help.  Some basic
> solution would be trigger which writes previous version of record
> to some other table. Trigger however can be disabled or removed (crazy
> solution would be to provide pernament
> triggers and tables which  can only be pernamently inserted).
> Then we have also possibility to modify tablespace directly on disk.
>
> But Postgres has ability to not override records when two concurrent
> transaction modify data to provide MVCC.
>
> So what about pernamently not vacuumable tables. Adding some xid log
> tables with hash of record on hash on previous hash.
> I think that would be serious additional advantage for best open source
> relational databes.
>
> Best regards,
>Marek Mosiewicz
>

What you are describing sounds like the "system versioning" flavor of
"temporal" tables. It's a part of the SQL Standard, but PostgreSQL has yet
to implement it in core. Basically, every row has a start_timestamp and
end_timestamp field. Updating a row sets the end_timestamp of the old
version and inserts a new one with a start_timestamp matching the
end-timestamp of the previous row. Once a record has a non-null [1]
end_timestamp, it is not possible to update that row via SQL. Regular SQL
statements effectively have a "AND end_timestamp IS NULL" filter on them,
so the old rows are not visible without specifically invoking temporal
features to get point-in-time queries. At the implementation level, this
probably means a table with 2 partitions, one for live rows all having null
end_timestamps, and one for archived rows which is effectively append-only.

This strategy is common practice for chain of custody and auditing
purposes, either as a feature of the RDBMS or home-rolled. I have also seen
it used for developing forecasting models (ex "what would this model have
told us to do if we had run it a year ago?").

A few years ago, I personally thought about implementing a hash-chain
feature, but my research at the time concluded that:

* Few customers were interested in going beyond what was required for
regulatory compliance
* Once compliant, any divergence from established procedures, even if it
was an unambiguous improvement, only invited re-examination of it and
adjacent procedures, and they would avoid that
* They could get the same validation by comparing against a secured backup
and out-of-band audit "logs" (most would call them "reports")
* They were of the opinion that if a bad actor got admin access, it was
"game over" anyway

The world may have changed since then, but even if there is now interest, I
wonder if that isn't better implemented at the OS level rather than the
RDBMS level.

 [1] some implementations don't use null, they use an end-timestamp set to
a date implausibly far in the future ( 3999-12-31 for example ), but the
concept remains that once the column is set to a real timestamp, the row
isn't visible to update statements.

Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size

[Corey Huinker]

On Thu, Feb 23, 2023 at 2:39 PM Andres Freund  wrote:

> Hi,
>
> On 2023-02-23 13:56:56 -0500, Tom Lane wrote:
> > Corey Huinker  writes:
> > > My not-ready-for-16 work on CAST( ... ON DEFAULT ... ) involved making
> > > FuncExpr/IoCoerceExpr/ArrayCoerceExpr have a safe_mode flag, and that
> > > necessitates adding a reserror boolean to ExprEvalStep for subsequent
> steps
> > > to test if the error happened.
> >
> > Why do you want it in ExprEvalStep ... couldn't it be in ExprState?
> > I can't see why you'd need more than one at a time during evaluation.
>
> I don't know exactly what CAST( ... ON DEFAULT ... ) is aiming for - I
> guess
> it wants to assign a different value when the cast fails?  Is the default
> expression a constant, or does it need to be runtime evaluated?  If a
> const,
> then the cast steps just could assign the new value. If runtime evaluation
> is
> needed I'd expect the various coerce steps to jump to the value
> implementing
> the default expression in case of a failure.
>

The default expression is itself a cast expression. So CAST (expr1 AS
some_type DEFAULT expr2 ON ERROR) would basically be a safe-mode cast of
expr1 to some_type, and only upon failure would the non-safe cast of expr2
to some_type be executed. Granted, the most common use case would be for
expr2 to be a constant or something that folds into a constant, but the
proposed spec allows for it.

My implementation involved adding a setting to CoalesceExpr that tested for
error flags rather than null flags, hence putting it in ExprEvalStep and
ExprState (perhaps mistakenly). Copying and adapting EEOP_JUMP_IF_NOT_NULL
lead me to this:

  EEO_CASE(EEOP_JUMP_IF_NOT_ERROR)
  {
  /* Transfer control if current result is non-error */
  if (!*op->reserror)
  {
  *op->reserror = false;
  EEO_JUMP(op->d.jump.jumpdone);
  }

  /* reset error flag */
  *op->reserror = false;

  EEO_NEXT();
  }

Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size

[Corey Huinker]

On Wed, Feb 22, 2023 at 5:47 PM Andres Freund  wrote:

> Hi,
>
> On 2023-02-22 16:34:44 -0500, Tom Lane wrote:
> > I wrote:
> > > Andres Freund  writes:
> > >> Maybe it's worth sticking a StaticAssert() for the struct size
> > >> somewhere.
> >
> > > Indeed.  I thought we had one already.
> >
> > >> I'm a bit wary about that being too noisy, there are some machines
> with
> > >> odd alignment requirements. Perhaps worth restricting the assertion to
> > >> x86-64 + armv8 or such?
> >
> > > I'd put it in first and only reconsider if it shows unfixable problems.
> >
> > Now that we've got the sizeof(ExprEvalStep) under control, shouldn't
> > we do the attached?
>
> Indeed. Pushed.
>
> Let's hope there's no rarely used architecture with odd alignment rules.
>
> Greetings,
>
> Andres Freund
>
>
>
I have a question about this that may affect some of my future work.

My not-ready-for-16 work on CAST( ... ON DEFAULT ... ) involved making
FuncExpr/IoCoerceExpr/ArrayCoerceExpr have a safe_mode flag, and that
necessitates adding a reserror boolean to ExprEvalStep for subsequent steps
to test if the error happened.

Will that change be throwing some architectures over the 64 byte count?

Re: Add SHELL_EXIT_CODE to psql

[Corey Huinker]

On Wed, Feb 22, 2023 at 4:18 PM Thomas Munro  wrote:

> On Tue, Jan 31, 2023 at 9:23 AM Corey Huinker 
> wrote:
> >> Unfortunately, there is a fail in FreeBSD
> https://cirrus-ci.com/task/6466749487382528
> >>
> >> Maybe, this patch is need to be rebased?
> >>
> >
> > That failure is in postgres_fdw, which this code doesn't touch.
> >
> > I'm not able to get to
> /tmp/cirrus-ci-build/build/testrun/postgres_fdw-running/regress/regression.out
> - It's not in either of the browseable zip files and the 2nd zip file isn't
> downloadable, so it's hard to see what's going on.
>
> Yeah, that'd be our current top flapping CI test[1].  These new
> "*-running" tests (like the old "make installcheck") are only enabled
> in the FreeBSD CI task currently, so that's why the failures only show
> up there.  I see[2] half a dozen failures of the "fdw_retry_check"
> thing in the past ~24 hours.
>
> [1]
> https://www.postgresql.org/message-id/flat/20221209001511.n3vqodxobqgscuil%40awork3.anarazel.de
> [2] http://cfbot.cputube.org/highlights/regress.html


Thanks for the explanation. I was baffled.

Re: Add SHELL_EXIT_CODE to psql

[Corey Huinker]

>
>
>
> The patch set seem to be in a good shape and pretty stable for quite a
> while.
> Could you add one more minor improvement, a new line after variables
> declaration?
>

As you wish. Attached.


>
> After this changes, I think, we make this patch RfC, shall we?
>
>
Fingers crossed.
From bb55e0fd1cd2de6fa25b7fc738dd6482d0c008c4 Mon Sep 17 00:00:00 2001
From: coreyhuinker 
Date: Wed, 11 Jan 2023 17:27:23 -0500
Subject: [PATCH v9 1/3] Add PG_OS_TARGET environment variable to enable
 OS-specific changes to regression tests.

---
 src/test/regress/pg_regress.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 6cd5998b9d..bf0ada4560 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -594,6 +594,17 @@ initialize_environment(void)
 #endif
 	}
 
+		/*
+		 * Regression tests that invoke OS commands must occasionally use
+		 * OS-specific syntax (example: NUL vs /dev/null). This env var allows
+		 * those tests to adjust commands accordingly.
+		 */
+#if defined(WIN32)
+		setenv("PG_OS_TARGET", "WIN32", 1);
+#else
+		setenv("PG_OS_TARGET", "OTHER", 1);
+#endif
+
 	/*
 	 * Set translation-related settings to English; otherwise psql will
 	 * produce translated messages and produce diffs.  (XXX If we ever support
-- 
2.39.2

From fe954f883f4ee65cbfe5dc2c20f012465d031ada Mon Sep 17 00:00:00 2001
From: coreyhuinker 
Date: Thu, 12 Jan 2023 23:04:31 -0500
Subject: [PATCH v9 2/3] Add wait_result_to_exit_code().

---
 src/common/wait_error.c | 15 +++
 src/include/port.h  |  1 +
 2 files changed, 16 insertions(+)

diff --git a/src/common/wait_error.c b/src/common/wait_error.c
index 4a3c3c61af..1eb7ff3fa2 100644
--- a/src/common/wait_error.c
+++ b/src/common/wait_error.c
@@ -127,3 +127,18 @@ wait_result_is_any_signal(int exit_status, bool include_command_not_found)
 		return true;
 	return false;
 }
+
+/*
+ * Return the POSIX exit code (0 to 255) that corresponds to the argument.
+ * The argument is a return code returned by wait(2) or waitpid(2), which
+ * also applies to pclose(3) and system(3).
+ */
+int
+wait_result_to_exit_code(int exit_status)
+{
+	if (WIFEXITED(exit_status))
+		return WEXITSTATUS(exit_status);
+	if (WIFSIGNALED(exit_status))
+		return WTERMSIG(exit_status);
+	return 0;
+}
diff --git a/src/include/port.h b/src/include/port.h
index e66193bed9..1b119a3c56 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -495,6 +495,7 @@ extern char *escape_single_quotes_ascii(const char *src);
 extern char *wait_result_to_str(int exitstatus);
 extern bool wait_result_is_signal(int exit_status, int signum);
 extern bool wait_result_is_any_signal(int exit_status, bool include_command_not_found);
+extern int wait_result_to_exit_code(int exit_status);
 
 /*
  * Interfaces that we assume all Unix system have.  We retain individual macros
-- 
2.39.2

From 0f91719c26b6201f4aaa436bd13141ba325e2f85 Mon Sep 17 00:00:00 2001
From: coreyhuinker 
Date: Wed, 22 Feb 2023 14:55:34 -0500
Subject: [PATCH v9 3/3] Add psql variables SHELL_ERROR and SHELL_EXIT_CODE
 which report the success or failure of the most recent psql OS-command
 executed via the \! command or `backticks`. These variables are roughly
 analogous to ERROR and SQLSTATE, but for OS level operations instead of SQL
 commands.

---
 doc/src/sgml/ref/psql-ref.sgml | 21 ++
 src/bin/psql/command.c | 15 +
 src/bin/psql/help.c|  4 
 src/bin/psql/psqlscanslash.l   | 34 +-
 src/test/regress/expected/psql.out | 29 +
 src/test/regress/sql/psql.sql  | 25 ++
 6 files changed, 123 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc6528dc11..3d8a7353b3 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4264,6 +4264,27 @@ bar
 
   
 
+  
+   SHELL_ERROR
+   
+
+ true if the last shell command failed, false if
+ it succeeded. This applies to shell commands invoked via either \!
+ or `. See also SHELL_EXIT_CODE.
+
+   
+  
+
+  
+   SHELL_EXIT_CODE
+   
+
+ The exit code return by the last shell command, invoked via either \! or `.
+ 0 means no error. See also SHELL_ERROR.
+
+   
+  
+
   
 SHOW_ALL_RESULTS
 
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 955397ee9d..40ba2810ea 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5032,6 +5032,21 @@ do_shell(const char *command)
 	else
 		result = system(command);
 
+	if (result == 0)
+	{
+		SetVariable(pset.vars, "SHELL_EXIT_CODE", "0");
+		SetVariable(pset.vars, "SHELL_ERROR", "false");
+	}
+	else
+	{
+		int		exit_code = wait_result_to_exit_code(result);
+		char	buf[32];
+
+		

Re: psql memory leaks

[Corey Huinker]

On Mon, Feb 20, 2023 at 9:56 PM Kyotaro Horiguchi 
wrote:

> I noticed that \bind is leaking memory for each option.
>
> =# SELECT $1, $2, $3 \ bind 1 2 3 \g
>
> The leaked memory blocks are comming from
> psql_scan_slash_option(). The attached small patch resolves that
> issue.  I looked through the function's call sites, but I didn't find
> the same mistake.
>
> regards.
>
>
Good catch. Patch passes make check-world.

Re: Improving inferred query column names

[Corey Huinker]

On Sat, Feb 11, 2023 at 3:47 PM Vladimir Churyukin 
wrote:

> For backwards compatibility I guess you can have a GUC flag controlling
> that behavior that can be set into backwards compatibility mode if required.
> The previous functionality can be declared deprecated and removed (with
> the flag) once the current version becomes unsupported.
>

Seems more like a per-session setting than a GUC.

Here's a suggestion off the top of my head.

We create a session setting inferred_column_name_template.

The template takes a formatting directive %N which is just a counter

SET inferred_column_name_template = 'col_%N'


which would give you col_1, col_2, regardless of what kind of expression
the columns were

We could introduce another directive, %T

SET inferred_column_name_template = '%T_%N'


which prints the datatype short name of the column. In this case, %N would
increment per datatype, so text_1, integer_1, text_2, timestamptz_1, text_3

Getting fancier, we could introduce something less datatype centric, %F

SET inferred_column_name_template = '%F_%N'


Which would walk the following waterfall and stop on the first match

   1. The datatype short name if the expression is explicitly casted
(either CAST or ::)
   2. the name of the function if the outermost expression was a function
(aggregate, window, or scalar),  so sum_1, substr_1
   3. 'case' if the outermost expression was  case
   4. 'expr' if the expression was effectively an operator (  SELECT 3+4,
'a'  || 'b' etc)
   5. the datatype short name for anything that doesn't match any of the
previous, and for explicit casts


Keeping track of all the %N counters could get silly, so maybe a %P which
is simply the numeric column position of the column, so your result set
would go like:  id, name, col_3, last_login, col_5.

We would have to account for the case where the user left either %N or %P
out of the template, so one of them would be an implied suffix if both were
absent, or we maybe go with

SET inferred_column_name_prefix = '%F_';
SET inferred_column_name_counter = 'position';   /* position, counter,
per_type_counter */

Or we just cook up a few predefined naming schemes, and let the user pick
from those.

One caution I have is that I have seen several enterprise app database
designs that have lots of user-customizable columns with names like
varchar1, numeric4, etc. Presumably the user would know their environment
and not pick a confusing template.

Re: appendBinaryStringInfo stuff

[Corey Huinker]

On Fri, Feb 10, 2023 at 7:16 AM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:

> On 19.12.22 07:13, Peter Eisentraut wrote:
> > Also, the argument type of appendBinaryStringInfo() is char *.  There is
> > some code that uses this function to assemble some kind of packed binary
> > layout, which requires a bunch of casts because of this.  I think
> > functions taking binary data plus length should take void * instead,
> > like memcpy() for example.
>
> I found a little follow-up for this one: Make the same change to
> pq_sendbytes(), which is a thin wrapper around appendBinaryStringInfo().
>   This would allow getting rid of further casts at call sites.
>

+1

Has all the benefits that 54a177a948b0a773c25c6737d1cc3cc49222a526 had.

Passes make check-world.