Re: cache lookup failed when \d t concurrent with DML change column data type
On Fri, 25 Oct 2024 at 09:51, Andrei Lepikhov wrote: > > It may be have made sense to lock the row of replaced index in pg_class > and pg_index until the transaction, altered it will be commmitted. But, > because ALTER TABLE is not fully MVCC-safe, it may be expected (or > acceptable) behaviour. I suspect this is the case. If that is, should be reflect it in the doc? -- Best regards, Kirill Reshke
Forbid to DROP temp tables of other sessions
Hi, I noticed that TRUNCATE and ALTER commands on temporary tables of other sessions produce an error "cannot truncate/alter temporary tables of other sessions". But are there any reasons to allow us to DROP such tables? It seems to me that the only case when we may need it is the removal of orphan tables. But the autovacuum is responsible for this and it uses a different functionality. I'm wondering if there are any other cases. If not, can we just handle it for example in ExecDropStmt and produce an error like "cannot drop temporary tables of other sessions"? -- Best regards, Daniil Davydov
Re: proposal: schema variables
On Fri, 2024-10-25 at 07:21 +0200, Pavel Stehule wrote:
> > > + elog(DEBUG1, "pg_session_variables start");
> >
> > I don't think that message is necessary, particularly with DEBUG1.
> > I have removed this message and the "end" message as well.
>
> removed
Thanks.
> > > + memset(values, 0, sizeof(values));
> > > + memset(nulls, 0, sizeof(nulls));
> >
> > Instead of explicitly zeroing out the arrays, I have used an empty
> > initializer
> > in the definition, like
> >
> > bool nulls[NUM_PG_SESSION_VARIABLES_ATTS] = {};
> >
> > That should have the same effect.
> > If you don't like that, I have no real problem with your original code.
>
> I prefer the original way - minimally it is a common pattern. I didn't find
> any usage of `= {} ` in code
That's alright by me.
> > > + values[0] = ObjectIdGetDatum(svar->varid);
> > > + values[3] = ObjectIdGetDatum(svar->typid);
> >
> > You are using the type ID without checking if it exists in the catalog.
> > I think that is a bug.
>
> The original idea was using typid as hint identification of deleted
> variables. The possibility
> that this id will not be consistent for the current catalogue was expected.
> And it
> is a reason why the result type is just Oid and not regtype. Without it,
> pg_session_variables
> shows just empty rows (except oid) for dropped not yet purged variables.
I see your point. It is for testing and debugging only.
>
> owing typid has some information value, but I don't think it is absolutely
> necessary. I see some possible changes:
>
> 1. no change
> 2. remove typid column
> 3. show typid only when variable is valid, and using regtype as output type,
> remove typname
>
> What do you prefer?
I'd say leave it as it is. I agree that it is not dangerous, and if it is
intentional that
non-existing type IDs might be displayed, I have no problem with it.
Yours,
Laurenz Albe
Re: Refactor to use common function 'get_publications_str'.
On Thu, Oct 24, 2024 at 5:44 PM Peter Smith wrote: > > On Thu, Oct 24, 2024 at 5:41 PM Michael Paquier wrote: > > > > On Thu, Oct 24, 2024 at 05:27:25PM +1100, Peter Smith wrote: > > > Yes, well spotted -- I was aware of that. Originally I had coded a >= > > > PG15 check for that pub_names assignment. e.g. > > > > > > if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 15) > > > GetPublicationsStr(MySubscription->publications, pub_names, true); > > > > I was wondering about putting the call of GetPublicationsStr() in the > > first block with the makeStringInfo(), have an Assert checking that > > pub_names->data is never NULL in the second block, and destroy the > > StringInfo only if it has been allocated. > > > > > But, somehow it seemed messy to do that just to cater for something I > > > thought was not particularly likely. OTOH, I am happy to put that > > > check back in if you think it is warranted. > > > > I think I would add it. The publication list is not mandatory, but > > your patch makes it look so. > > -- > > No problem. I will post an updated patch tomorrow. > I've attached the patch v4. == Kind Regards, Peter Smith. Fujitsu Australia v4-0001-Refactor-to-use-common-function-GetPublicationsSt.patch Description: Binary data
Re: general purpose array_sort
On Wed, Oct 23, 2024 at 10:28 PM Junwang Zhao wrote:
> PFA v7 with multi-array support.
>
if (ARR_NDIM(array) == 1)
{
}
else
{
}
can be simplified.
i think beginning part of array_sort can be like the following:
(newline emitted)
-
if (ARR_NDIM(array) < 1)
PG_RETURN_ARRAYTYPE_P(array);
if (dirstr != NULL)
{
if (!parse_sort_order(text_to_cstring(dirstr), &sort_asc, &nulls_first))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("second parameter must be a valid sort
direction")));
}
elmtyp = ARR_ELEMTYPE(array);
if (ARR_NDIM(array) > 1)
elmtyp = get_array_type(elmtyp);
typentry = (TypeCacheEntry *) fcinfo->flinfo->fn_extra;
if (typentry == NULL || typentry->type_id != elmtyp)
{
typentry = lookup_type_cache(elmtyp, sort_asc ?
TYPECACHE_LT_OPR : TYPECACHE_GT_OPR);
if ((sort_asc && !OidIsValid(typentry->lt_opr)) ||
(!sort_asc && !OidIsValid(typentry->gt_opr)))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_FUNCTION),
errmsg("could not identify an ordering operator
for type %s",
format_type_be(elmtyp;
fcinfo->flinfo->fn_extra = (void *) typentry;
}
-
/*
* array_sort
*
* Sorts the array in either ascending or descending order.
* The array must be empty or one-dimensional.
*/
comments need to be updated.
typedef enum
PARSE_SORT_ORDER_DONE
} ParseSortOrderState;
last one, should have comma, like
"PARSE_SORT_ORDER_DONE, "
Re: Pgoutput not capturing the generated columns
On Thu, 24 Oct 2024 at 16:44, Amit Kapila wrote:
>
> On Thu, Oct 24, 2024 at 12:15 PM vignesh C wrote:
> >
> > The attached v41 version patch has the changes for the same.
> >
>
> Please find comments for the new version as follows:
> 1.
> + Generated columns may be skipped during logical replication
> according to the
> + CREATE PUBLICATION option
> + linkend="sql-createpublication-params-with-publish-generated-columns">
> + include_generated_columns.
>
> The above statement doesn't sound to be clear. Can we change it to:
> "Generated columns are allowed to be replicated during logical
> replication according to the CREATE PUBLICATION
> option .."?
Modified
> 2.
> static void publication_invalidation_cb(Datum arg, int cacheid,
> uint32 hashvalue);
> -static void send_relation_and_attrs(Relation relation, TransactionId xid,
> - LogicalDecodingContext *ctx,
> - Bitmapset *columns);
> static void send_repl_origin(LogicalDecodingContext *ctx,
> ...
> ...
> static RelationSyncEntry *get_rel_sync_entry(PGOutputData *data,
> Relation relation);
> +static void send_relation_and_attrs(Relation relation, TransactionId xid,
> + LogicalDecodingContext *ctx,
> + RelationSyncEntry *relentry);
>
> Why the declaration of this function is changed?
Two changes were made: a) The function declaration need to be moved
down as the RelationSyncEntry structure is defined below. b) Bitmapset
was replaced with RelationSyncEntry to give send_relation_and_attrs
access to RelationSyncEntry.pubgencols and RelationSyncEntry.columns.
Instead of adding a new parameter to the function, RelationSyncEntry
was utilized, as it contains both pubgencols and columns members.
> 3.
> + /*
> + * Skip publishing generated columns if the option is not specified or
> + * if they are not included in the column list.
> + */
> + if (att->attgenerated && !relentry->pubgencols && !columns)
>
> In the comment above, shouldn't "specified or" be "specified and"?
Modified
> 4.
> +pgoutput_pubgencol_init(PGOutputData *data, List *publications,
> + RelationSyncEntry *entry)
>
> {
> ...
> + foreach(lc, publications)
> + {
> + Publication *pub = lfirst(lc);
> +
> + /* No need to check column list publications */
> + if (is_column_list_publication(pub, entry->publish_as_relid))
>
> Are we ignoring column_list publications because for such publications
> the value of column_list prevails and we ignore
> 'publish_generated_columns' value? If so, it is not clear from the
> comments.
Yes column takes precedence over publish_generated_columns value, so
column list publications are skipped. Modified the comments
accordingly.
> 5.
> /* Initialize the column list */
> pgoutput_column_list_init(data, rel_publications, entry);
> +
> + /* Initialize publish generated columns value */
> + pgoutput_pubgencol_init(data, rel_publications, entry);
> +
> + /*
> + * Check if there is conflict with the columns selected for the
> + * publication.
> + */
> + check_conflicting_columns(rel_publications, entry);
> }
>
> It looks odd to check conflicting column lists among publications
> twice once in pgoutput_column_list_init() and then in
> check_conflicting_columns(). Can we merge those?
Modified it to check from pgoutput_column_list_init
The v42 version patch attached at [1] has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CALDaNm2wFZRzSJLcNi_uMZcSUWuZ8%2Bkktc0n3Nfw9Fdti9WbVA%40mail.gmail.com
Regards,
Vignesh
Re: Useless field ispartitioned in CreateStmtContext
On Thu, 24 Oct 2024 at 19:23, Alena Rybakina wrote: > > Hi! > > On 24.10.2024 16:07, hugo wrote: > > Hi! > >When looking at the partition-related code, I found that the > ispartitioned > > field in CreateStmtContext is not used. It looks like we can safely remove it > and > > avoid invalid assignment logic. > > > > Here's a very simple fix， any suggestion? > > > > diff --git a/src/backend/parser/parse_utilcmd.c > b/src/backend/parser/parse_utilcmd.c > > index 1e15ce10b48..6dea85cc2dc 100644 > > --- a/src/backend/parser/parse_utilcmd.c > > +++ b/src/backend/parser/parse_utilcmd.c > > @@ -89,7 +89,6 @@ typedef struct > > List *alist; /* "after list" of things to > do after creating > > * the table > */ > > IndexStmt *pkey; /* PRIMARY KEY index, if any > */ > > - boolispartitioned; /* true if table is partitioned */ > > PartitionBoundSpec *partbound; /* transformed FOR VALUES */ > > boolofType; /* true if statement contains > OF typename */ > > } CreateStmtContext; > > @@ -246,7 +245,6 @@ transformCreateStmt(CreateStmt *stmt, const char > *queryString) > > cxt.blist = NIL; > > cxt.alist = NIL; > > cxt.pkey = NULL; > > - cxt.ispartitioned = stmt->partspec != NULL; > > cxt.partbound = stmt->partbound; > > cxt.ofType = (stmt->ofTypename != NULL); > > @@ -3401,7 +3399,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt, > > cxt.blist = NIL; > > cxt.alist = NIL; > > cxt.pkey = NULL; > > - cxt.ispartitioned = (rel->rd_rel->relkind == > RELKIND_PARTITIONED_TABLE); > > cxt.partbound = NULL; > > cxt.ofType = false; > > > > I absolutely agree with you. I found that ispartitioned parameter has been > defined but it is never used during optimization. > > I also noticed that its local copy is being redefined in the > ATExecDropIdentity, ATExecSetIdentity and ATExecAddIdentity functions. > > So, I'm willing to agree with you. > > BTW, the regression tests were successful. > > -- > Regards, > Alena Rybakina > Postgres Professional Hi all. This field was introduced by f0e4475 It was useful for various checks [1] but after we allowed unique keys on partition tables in commit eb7ed3f [2], this field became unneeded. CreateStmtContext is internal parser struct, so no not used outside PG core (i mean, by any extension). So, my suggestion is to remove it. Hugo, can you please create a CF entry for this patch? Patch itself looks good. [1] https://github.com/postgres/postgres/blob/f0e44751d7175fa3394da2c8f85e3ceb3cdbfe63/src/backend/parser/parse_utilcmd.c#L617 [2] https://github.com/postgres/postgres/commit/eb7ed3f3063401496e4aa4bd68fa33f0be31a72f#diff-5bd59ecc8991bacaefd56f7fe986287b8d664e62566eb3588c3845d7625cacf1L715 -- Best regards, Kirill Reshke
Re: Docs Build in CI failing with "failed to load external entity"
Hi, On 2024-10-25 08:22:42 +0300, Thomas Munro wrote: > On Fri, Oct 25, 2024 at 4:44 AM Tom Lane wrote: > > Melanie Plageman writes: > > > I know in the past docs builds failing with "failed to load external > > > entity" have happened on macos. But, recently I've noticed this > > > failure for docs build on CI (not on macos) -- docs build is one of > > > the jobs run under the "Compiler Warnings" task. > > > > It looks to me like a broken docbook installation on (one of?) > > the CI machines. Note that the *first* complaint is > > > > [19:23:20.590] file:///etc/xml/catalog:1: parser error : Document is empty > > Yeah. That CI job runs on a canned Debian image that is rebuilt and > republished every couple of days to make sure it's using up to date > packages and kernel etc. Perhaps the package installation silently > corrupted /etc/xml/catalog, given that multiple packages probably mess > with it, though I don't have a specific theory for how that could > happen, given that package installation seems to be serial... The > installation log doesn't seem to show anything suspicious. Yea, it's clearly corrupted - the file is empty. I don't understand how that can happen, particularly without any visible error. I certainly can't reproduce it when installing the packages exactly the same way it happens for the image. I also don't think this happened before, despite the recipe for building the images not having meaningfully changed in quite a while. So it must be some rare edge case. > I wonder if this will magically fix itself when the next CI image > build cron job kicks off. I have no idea what time zone this page is > showing but it should happen in another day or so, unless Andres is > around to kick it sooner: > > https://cirrus-ci.com/github/anarazel/pg-vm-images I did trigger a rebuild of the image just now. Hopefully that'll fix it. Greetings, Andres Freund
Re: cache lookup failed when \d t concurrent with DML change column data type
On 10/25/24 14:15, Kirill Reshke wrote: On Fri, 25 Oct 2024 at 09:51, Andrei Lepikhov wrote: It may be have made sense to lock the row of replaced index in pg_class and pg_index until the transaction, altered it will be commmitted. But, because ALTER TABLE is not fully MVCC-safe, it may be expected (or acceptable) behaviour. I suspect this is the case. If that is, should be reflect it in the doc? We already have the doc entry on such cases [1]. This is a suitable place to change if someone wants to detail this 'failed cache lookup' case. [1] https://www.postgresql.org/docs/16/mvcc-caveats.html -- regards, Andrei Lepikhov
Re: Trigger more frequent autovacuums of heavy insert tables
I really appreciate all the work to make vacuum better. Anything that helps
our problem of autovacuum not scaling well for large tables is a win.
I'm not overly familiar with this part of the code base, but here are some
questions/ideas:
+ /*
+* Every block marked all-frozen in the VM must also be marked
+* all-visible.
+*/
+ if (new_rel_allfrozen > new_rel_allvisible)
+ new_rel_allfrozen = new_rel_allvisible;
+
Maybe tweak either the comment, or the code, as I read that comment as
meaning:
if (new_rel_allfrozen > new_rel_allvisible)
new_ral_allvisible = new_rel_allfrozen;
+ /*
+* If we are modifying relallvisible manually, it
is not clear
+* what relallfrozen value would make sense.
Therefore, set it to
+* -1, or unknown. It will be updated the next time
these fields
+* are updated.
+*/
+ replaces[ncols] = Anum_pg_class_relallfrozen;
+ values[ncols] = Int32GetDatum(-1);
Do we need some extra checks later on when we are actually using this to
prevent negative numbers in the calculations? It's only going to make
pcnt_unfrozen something like 1.0001 but still might want to skip that.
In autovacuum.c, seems we could simplify some of the logic there to this?:
if (relpages > 0 && reltuples > 0) {
relallfrozen = classForm->relallfrozen;
relallvisible = classForm->relallvisible;
if (relallvisible > relpages)
relallvisible = relpages;
if (relallfrozen > relallvisible)
relallfrozen = relallvisible;
pcnt_unfrozen = 1 - ((float4) relallfrozen / relpages);
}
vacinsthresh = (float4) vac_ins_base_thresh + vac_ins_scale_factor *
reltuples * pcnt_unfrozen;
Again, I'm not clear under what circumstances will relallvisible > relpages?
Cheers,
Greg
Re: general purpose array_sort
On Thu, Oct 24, 2024 at 9:06 AM Tom Lane wrote: > This business with a textual representation of a sort clause seems like > over-engineering ... except that it's also under-engineered, because > the parsing is lame and incomplete. (No USING option, and the fact > that collation comes from somewhere else seems impossibly confusing.) > Let's drop that. > I can accept this outcome though an optional three-valued boolean sort order (ascending and descending only) I'd argue is worth keeping. null value placement too I guess, three-valued boolean (nulls_first). David J.
Re: general purpose array_sort
On Thu, Oct 24, 2024 at 8:40 PM jian he wrote:
>
> On Wed, Oct 23, 2024 at 10:28 PM Junwang Zhao wrote:
> > PFA v7 with multi-array support.
> >
>
> if (ARR_NDIM(array) == 1)
> {
> }
> else
> {
> }
> can be simplified.
> i think beginning part of array_sort can be like the following:
> (newline emitted)
>
> -
> if (ARR_NDIM(array) < 1)
> PG_RETURN_ARRAYTYPE_P(array);
> if (dirstr != NULL)
> {
> if (!parse_sort_order(text_to_cstring(dirstr), &sort_asc,
> &nulls_first))
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("second parameter must be a valid sort
> direction")));
> }
> elmtyp = ARR_ELEMTYPE(array);
> if (ARR_NDIM(array) > 1)
> elmtyp = get_array_type(elmtyp);
I'm wondering should we cache the type entry for both element type and
the corresponding array type, for example if we have a table:
create table t(b int[]);
insert into t values ('{1,3}'),('{{2,3}}'),('{{1,2},{0,2}}');
with 1-d array and m-d array interleaved, then the following query will
call lookup_type_cache multiple times.
select array_sort((t.b)) from t;
> typentry = (TypeCacheEntry *) fcinfo->flinfo->fn_extra;
> if (typentry == NULL || typentry->type_id != elmtyp)
> {
> typentry = lookup_type_cache(elmtyp, sort_asc ?
> TYPECACHE_LT_OPR : TYPECACHE_GT_OPR);
> if ((sort_asc && !OidIsValid(typentry->lt_opr)) ||
> (!sort_asc && !OidIsValid(typentry->gt_opr)))
> ereport(ERROR,
> (errcode(ERRCODE_UNDEFINED_FUNCTION),
> errmsg("could not identify an ordering operator
> for type %s",
> format_type_be(elmtyp;
> fcinfo->flinfo->fn_extra = (void *) typentry;
> }
> -
> /*
> * array_sort
> *
> * Sorts the array in either ascending or descending order.
> * The array must be empty or one-dimensional.
> */
> comments need to be updated.
will fix it in the next version of patch.
>
>
> typedef enum
> PARSE_SORT_ORDER_DONE
> } ParseSortOrderState;
>
> last one, should have comma, like
> "PARSE_SORT_ORDER_DONE, "
will fix it.
--
Regards
Junwang Zhao
Re: Using read_stream in index vacuum
> On 24 Oct 2024, at 10:15, Andrey M. Borodin wrote: > > I've also added GiST vacuum to the patchset. I decided to add up a SP-GiST while having launched on pgconf.eu. Best regards, Andrey Borodin. v7-0001-Prototype-B-tree-vacuum-streamlineing.patch Description: Binary data v7-0002-Use-read_stream-in-GiST-vacuum.patch Description: Binary data v7-0003-Use-read_stream-in-SP-GiST-vacuum.patch Description: Binary data
Re: Refactor GetLockStatusData() by skipping unused backends and groups
On 2024/10/24 11:12, Bertrand Drouvot wrote: So, probably we can consider that this check is already in place. If it’s still worth adding, perhaps placing it inside the FAST_PATH_SLOT() macro could be an option... Or current assertion check is enough? Thought? Given that it's already done in FAST_PATH_GET_BITS(), I think that's fine as it is and v2 LGTM. Thanks for the review! Pushed. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Use function smgrclose() to replace the loop
Junwang Zhao 于2024年10月15日周二 18:56写道： > On Mon, Oct 14, 2024 at 6:30 PM Ilia Evdokimov > wrote: > > > > > > On 14.08.2024 09:32, Steven Niu wrote: > > > Hi, Kirill, Junwang, > > > > > > I made this patch to address the refactor issue in our previous email > > > discussion. > > > > https://www.postgresql.org/message-id/flat/CABBtG=cdtcbdcbk7mcsy6bjr3s5xutog0vsffuw8olduyyc...@mail.gmail.com > > > > > > > > > That is, the for loop in function smgrdestroy() and smgrdounlinkall > > > can be replaced with smgrclose(). > > > > > > for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) > > > smgrsw[reln->smgr_which].smgr_close(reln, forknum); > > > --> > > > smgrclose(rels[i]); > > > > > > Please let me know if you have any questions. > > > > > > Best Regards, > > > Steven from Highgo.com > > > > Hello, > > > > Are you sure we can refactor loop by 'smgrclose()'? I see it is not > > quite the same. > > smgrclose does more by setting smgr_cached_nblocks[] and smgr_targblock > to InvalidBlockNumber, I see no harm with the replacement, not 100% sure > though. > > > > > Regards, > > Ilia Evdokimov, > > Tantor Labs LLC. > > > > > -- > Regards > Junwang Zhao > When I look into smgrrelease(), which also resets smgr_cached_nblocks and smgr_targblock, I would say this is of good style. So a bare loop of calling smgr_close() without other cleanup actions is kind of weird to me. Unless there is some reason for the current code, I'd like to replace it. Regards, Steven
Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails
Alvaro Herrera 于2024年10月25日周五 16:30写道：
> On 2024-Oct-25, Tender Wang wrote:
>
> > Thanks for reporting. I can reproduce this memory invalid access on HEAD.
> > After debuging codes, I found the root cause.
> > In DetachPartitionFinalize(), below code:
> > att = TupleDescAttr(RelationGetDescr(partRel),
> >attmap->attnums[conkey[i] - 1] - 1);
> >
> > We should use confkey[i] -1 not conkey[i] - 1;
>
> Wow, how embarrasing, now that's one _really_ stupid bug and it's
> totally my own. Thanks for the analysis! I'll get this patched soon.
>
>
Hi,
When I debug codes, I find that the way to access AttrMap almost uses
"attrmp_ptr->attnums[offset]."
The codes now usually don't check if the offset is out of bounds, which
seems unsafe.
Can we wrap an access function? For example:
inline AttrNumber(attrmap_ptr, offset)
{
Assert(offset >= 0 && offset < attrmap_ptr->maplen);
return attrmap_ptr->attnums[offset];
}
Or a Macro.
I'm not sure if it's worth doing this.
--
Thanks,
Tender Wang
Re: Inconsistent use of relpages = -1
On Thu, 2024-10-24 at 05:01 +0300, Laurenz Albe wrote: > What you write above indicates that "relpages" = 0 and "reltuples" > > 0 > would also be acceptable. As Tom pointed out, that creates a risk that it's interpreted as infinite tuple denisity. The only functional change in my patch is to create the partitioned table with relpages=-1 to be more consistent with the value after it's analyzed. Regards, Jeff Davis
Re: altering a column's collation leaves an invalid foreign key
On Fri, Oct 25, 2024 at 2:27 PM jian he wrote:
>
> /*
> * ri_GenerateQualCollation --- add a COLLATE spec to a WHERE clause
> *
> - * At present, we intentionally do not use this function for RI queries that
> - * compare a variable to a $n parameter. Since parameter symbols always have
> - * default collation, the effect will be to use the variable's collation.
> - * Now that is only strictly correct when testing the referenced column,
> since
> - * the SQL standard specifies that RI comparisons should use the referenced
> - * column's collation. However, so long as all collations have the same
> - * notion of equality (which they do, because texteq reduces to bitwise
> - * equality), there's no visible semantic impact from using the referencing
> - * column's collation when testing it, and this is a good thing to do because
> - * it lets us use a normal index on the referencing column. However, we do
> - * have to use this function when directly comparing the referencing and
> - * referenced columns, if they are of different collations; else the parser
> - * will fail to resolve the collation to use.
> + * We only have to use this function when directly comparing the referencing
> + * and referenced columns, if they are of different collations; else the
> + * parser will fail to resolve the collation to use. We don't need to use
> + * this function for RI queries that compare a variable to a $n parameter.
> + * Since parameter symbols always have default collation, the effect will be
> + * to use the variable's collation.
> + *
> + * Note that we require that the collations of the referencing and the
> + * referenced column have the some notion of equality: Either they have to
> + * both be deterministic or else they both have to be the same.
> */
drop table if exists pktable, fktable;
CREATE TABLE pktable (x text COLLATE "POSIX" PRIMARY KEY);
CREATE TABLE fktable (x text COLLATE "C" REFERENCES pktable on update
cascade on delete cascade);
INSERT INTO pktable VALUES ('A'), ('Å');
INSERT INTO fktable VALUES ('A');
update pktable set x = 'a' collate "C" where x = 'A' collate "POSIX";
the cascade update fktable query string is:
UPDATE ONLY "public"."fktable" SET "x" = $1 WHERE $2 OPERATOR(pg_catalog.=) "x"
ideally it should be
UPDATE ONLY "public"."fktable" SET "x" = $1 collate "C" WHERE $2
collate "POSIX" OPERATOR(pg_catalog.=) "x"
as we already mentioned in several places: PK-FK tie either they have to
both be deterministic or else they both have to be the same collation
oid.
so the reduction to
UPDATE ONLY "public"."fktable" SET "x" = $1 WHERE $2 OPERATOR(pg_catalog.=) "x"
is safe.
but you look at SPI_execute_snapshot, _SPI_convert_params. then we can see
the collation metadata is not keeped.
> + We don't need to use
> + * this function for RI queries that compare a variable to a $n parameter.
> + * Since parameter symbols always have default collation, the effect will be
> + * to use the variable's collation.
so I think a better description is
> + We don't need to use
> + * this function for RI queries that compare a variable to a $n parameter.
> + * Since parameter symbols don't have collation information, the effect will
> be
> + * to use the variable's collation.
you can see related discovery in
https://www.postgresql.org/message-id/CACJufxEtPBWAk7nEn69ww2LKi9w1i4dLwd5gnjD1DQ2vaYoi2g%40mail.gmail.com
Re: Can rs_cindex be < 0 for bitmap heap scans?
On Fri, Oct 25, 2024 at 10:29 AM Melanie Plageman
wrote:
>
> Tom suggested off-list that if rs_cindex can't be zero, then it should
> be unsigned. I checked the other scan types using the
> HeapScanDescData, and it seems none of them use values of rs_cindex or
> rs_ntuples < 0. As such, here is a patch making both rs_ntuples and
> rs_cindex unsigned.
I realized one of the asserts was superfluous. Please find v3 attached.
- Melanie
From 262984fde17c1ae3ba220eb5533c8598fb55cae1 Mon Sep 17 00:00:00 2001
From: Melanie Plageman
Date: Fri, 25 Oct 2024 09:09:24 -0400
Subject: [PATCH v3] Make rs_cindex and rs_ntuples unsigned
HeapScanDescData.rs_cindex and rs_ntuples can't be less than 0. All scan
types using the heap scan descriptor expect these values to be >= 0.
Make that expectation clear by making rs_cindex and rs_ntuples unsigned.
Also remove the test in heapam_scan_bitmap_next_tuple() that checks if
rs_cindex < 0. This was never true, but now that rs_cindex is unsigned,
it makes even less sense.
While we are at it, initialize both rs_cindex and rs_ntuples to 0 in
initscan().
Author: Melanie Plageman
---
src/backend/access/heap/heapam.c | 7 +--
src/backend/access/heap/heapam_handler.c | 2 +-
src/include/access/heapam.h | 4 ++--
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 4c8febdc811..194f64c540d 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -378,6 +378,8 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
ItemPointerSetInvalid(&scan->rs_ctup.t_self);
scan->rs_cbuf = InvalidBuffer;
scan->rs_cblock = InvalidBlockNumber;
+ scan->rs_ntuples = 0;
+ scan->rs_cindex = 0;
/*
* Initialize to ForwardScanDirection because it is most common and
@@ -943,8 +945,8 @@ heapgettup_pagemode(HeapScanDesc scan,
{
HeapTuple tuple = &(scan->rs_ctup);
Page page;
- int lineindex;
- int linesleft;
+ uint32 lineindex;
+ uint32 linesleft;
if (likely(scan->rs_inited))
{
@@ -989,6 +991,7 @@ continue_page:
ItemId lpp;
OffsetNumber lineoff;
+ Assert(lineindex <= scan->rs_ntuples);
lineoff = scan->rs_vistuples[lineindex];
lpp = PageGetItemId(page, lineoff);
Assert(ItemIdIsNormal(lpp));
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 8c59b77b64f..82668fab19a 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2269,7 +2269,7 @@ heapam_scan_bitmap_next_tuple(TableScanDesc scan,
/*
* Out of range? If so, nothing more to look at on this page
*/
- if (hscan->rs_cindex < 0 || hscan->rs_cindex >= hscan->rs_ntuples)
+ if (hscan->rs_cindex >= hscan->rs_ntuples)
return false;
targoffset = hscan->rs_vistuples[hscan->rs_cindex];
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index b951466ced2..21c7b70edf2 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -103,8 +103,8 @@ typedef struct HeapScanDescData
int rs_empty_tuples_pending;
/* these fields only used in page-at-a-time mode and for bitmap scans */
- int rs_cindex; /* current tuple's index in vistuples */
- int rs_ntuples; /* number of visible tuples on page */
+ uint32 rs_cindex; /* current tuple's index in vistuples */
+ uint32 rs_ntuples; /* number of visible tuples on page */
OffsetNumber rs_vistuples[MaxHeapTuplesPerPage]; /* their offsets */
} HeapScanDescData;
typedef struct HeapScanDescData *HeapScanDesc;
--
2.34.1
Re: simplify regular expression locale global variables
On 10/15/24 8:12 AM, Peter Eisentraut wrote: We currently have static PG_Locale_Strategy pg_regex_strategy; static pg_locale_t pg_regex_locale; static Oid pg_regex_collation; but after the recent improvements to pg_locale_t handling, we don't need all three anymore. All the information we have is contained in pg_locale_t, so we just need to keep that one. This allows us to structure the locale-using regular expression code more similar to other locale-using code, mainly by provider, avoiding another layer that is specific only to the regular expression code. The first patch implements that. Jeff Davis has a patch which also fixes this while refactoring other stuff too which I prefer over your patch since it also cleans up the collation code in general. https://www.postgresql.org/message-id/2830211e1b6e6a2e26d845780b03e125281ea17b.camel%40j-davis.com The second patch removes a call to pg_set_regex_collation() that I think is unnecessary. The third patch adds a pg_unset_regex_collation() call that undoes what pg_set_regex_collation() does. I mainly used this to verify the second patch, but maybe it's also useful on its own, not sure. (I don't have any plans to get rid of the remaining global variable. That would certainly be nice from an intellectual point of view, but fiddling this into the regular expression code looks quite messy. In any case, it's probably easier with one variable instead of three, if someone wants to try.) I have not looked at your other two patches yet. Andreas
Re: Docs Build in CI failing with "failed to load external entity"
On Fri, Oct 25, 2024 at 4:31 AM Andres Freund wrote: > > On 2024-10-25 04:14:03 -0400, Andres Freund wrote: > > On 2024-10-25 08:22:42 +0300, Thomas Munro wrote: > > > I wonder if this will magically fix itself when the next CI image > > > build cron job kicks off. I have no idea what time zone this page is > > > showing but it should happen in another day or so, unless Andres is > > > around to kick it sooner: > > > > > > https://cirrus-ci.com/github/anarazel/pg-vm-images > > > > I did trigger a rebuild of the image just now. Hopefully that'll fix it. > > It did. I noticed that CI for my fork of Postgres, which had been failing on docs build and on test-running on the injection points test only on freebsd, started working as expected again this morning. All of this is a bit of magic to me -- are the CI images you build used by all of our CIs? - Melanie
Re: Docs Build in CI failing with "failed to load external entity"
On 2024-10-25 09:34:41 -0400, Melanie Plageman wrote: > I noticed that CI for my fork of Postgres, which had been failing on > docs build and on test-running on the injection points test only on > freebsd, started working as expected again this morning. All of this > is a bit of magic to me -- are the CI images you build used by all of > our CIs? Yes. Installing the packages every time would be far far too time consuming.
Re: Fix C23 compiler warning
On 2024-Oct-22, Peter Eisentraut wrote: > One thing I didn't realize until today is that currently C23 compilations > only work with meson. The autoconf version we are using doesn't support it, > and the configure results it produces are somehow faulty and then you get a > bunch of compilation errors. So if we wanted to make this a supported > thing, it looks like we would need to use at least autoconf 2.72. Oh wow. Should we at least update our autoconf requirement to 2.72 in master? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Digital and video cameras have this adjustment and film cameras don't for the same reason dogs and cats lick themselves: because they can." (Ken Rockwell)
Re: Trigger more frequent autovacuums of heavy insert tables
Thanks for the review!
On Thu, Oct 24, 2024 at 3:51 PM Greg Sabino Mullane wrote:
>
> I really appreciate all the work to make vacuum better. Anything that helps
> our problem of autovacuum not scaling well for large tables is a win.
>
> I'm not overly familiar with this part of the code base, but here are some
> questions/ideas:
>
> + /*
> +* Every block marked all-frozen in the VM must also be marked
> +* all-visible.
> +*/
> + if (new_rel_allfrozen > new_rel_allvisible)
> + new_rel_allfrozen = new_rel_allvisible;
> +
>
> Maybe tweak either the comment, or the code, as I read that comment as
> meaning:
>
> if (new_rel_allfrozen > new_rel_allvisible)
> new_ral_allvisible = new_rel_allfrozen;
I've updated it. An all-frozen block must also be all-visible. But not
all-visible blocks are all-frozen
> + /*
> +* If we are modifying relallvisible manually, it is
> not clear
> +* what relallfrozen value would make sense.
> Therefore, set it to
> +* -1, or unknown. It will be updated the next time
> these fields
> +* are updated.
> +*/
> + replaces[ncols] = Anum_pg_class_relallfrozen;
> + values[ncols] = Int32GetDatum(-1);
>
> Do we need some extra checks later on when we are actually using this to
> prevent negative numbers in the calculations? It's only going to make
> pcnt_unfrozen something like 1.0001 but still might want to skip that.
Great point! I've added this
> In autovacuum.c, seems we could simplify some of the logic there to this?:
>
> if (relpages > 0 && reltuples > 0) {
>
> relallfrozen = classForm->relallfrozen;
> relallvisible = classForm->relallvisible;
>
> if (relallvisible > relpages)
> relallvisible = relpages;
>
> if (relallfrozen > relallvisible)
> relallfrozen = relallvisible;
>
> pcnt_unfrozen = 1 - ((float4) relallfrozen / relpages);
>
> }
> vacinsthresh = (float4) vac_ins_base_thresh + vac_ins_scale_factor *
> reltuples * pcnt_unfrozen;
I've done something similar to this in attached v2.
> Again, I'm not clear under what circumstances will relallvisible > relpages?
I think this is mostly if someone manually updated the relation stats,
so we clamp it for safety.
- Melanie
From c2e29150e923f9782ce24a7a4e7d6f2d7445b543 Mon Sep 17 00:00:00 2001
From: Melanie Plageman
Date: Fri, 4 Oct 2024 17:06:04 -0400
Subject: [PATCH v3 2/2] Trigger more frequent autovacuums with relallfrozen
Calculate the insert threshold for triggering an autovacuum of a
relation based on the number of unfrozen pages. By only considering the
"active" (unfrozen) portion of the table when calculating how many
tuples to add to the insert threshold, we can trigger more frequent
vacuums of insert-heavy tables and increase the chances of vacuuming
those pages when they still reside in shared buffers.
Author: Melanie Plageman
Reviewed-by: Greg Sabino Mullane
---
doc/src/sgml/config.sgml | 4 +-
src/backend/postmaster/autovacuum.c | 37 ++-
src/backend/utils/misc/postgresql.conf.sample | 4 +-
3 files changed, 39 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index dc401087dc6..3bd22f7f1e7 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8710,10 +8710,10 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
-Specifies a fraction of the table size to add to
+Specifies a fraction of the active (unfrozen) table size to add to
autovacuum_vacuum_insert_threshold
when deciding whether to trigger a VACUUM.
-The default is 0.2 (20% of table size).
+The default is 0.2 (20% of active table size).
This parameter can only be set in the postgresql.conf
file or on the server command line;
but the setting can be overridden for individual tables by
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index dc3cf87abab..364b46f672d 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2922,7 +2922,12 @@ relation_needs_vacanalyze(Oid relid,
{
bool force_vacuum;
bool av_enabled;
- float4 reltuples; /* pg_class.reltuples */
+
+ /* From pg_class */
+ float4 reltuples;
+ int32 relpages;
+ int32 relallfrozen;
+ int32 relallvisible;
/* constants from reloptions or GUC variables */
int vac_base_thresh,
@@ -3030,6 +3035,10 @@ relation_needs_vacanalyze(Oid relid,
*/
if (PointerIsValid(tabentry) && AutoVacuumingActive())
{
+ float4 pcnt_unfrozen = 1;
+
+ relpages = classForm->relpages;
+ relallfrozen = classForm->relallfrozen;
reltuples = classForm->reltuples;
vactuples = tabentry->dead_tuples;
instuples = tabentry->ins_since_vacuum;
@@ -
Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails
On 2024-Oct-25, Alexander Lakhin wrote:
> I've also discovered another anomaly with a similar setup, but it's not
> related to 53af9491a.
Hmm, it may well be a preexisting problem, but I do think it involves
the same code. As far as I can tell, the value "2" here
> This script ends up with:
> ERROR: invalid attribute number 2
> ERROR: cache lookup failed for attribute 2 of relation 16398
is coming from riinfo->confdelsetcols which was set up by
DetachPartitionFinalize during the last DETACH operation.
> (Perhaps it deserves a separate discussion.)
No need for that.
A backtrace from the errfinish gives me this (apologies for the
wide terminal):
#0 errfinish (filename=0x5610d93e4510
"../../../../../pgsql/source/master/src/backend/parser/parse_relation.c",
lineno=3618,
funcname=0x5610d93e5518 <__func__.5> "attnumAttName") at
../../../../../../pgsql/source/master/src/backend/utils/error/elog.c:475
#1 0x5610d8d68bcc in attnumAttName (rd=rd@entry=0x7f69dd0c1a90, attid=2)
at ../../../../../pgsql/source/master/src/backend/parser/parse_relation.c:3618
#2 0x5610d924a8c5 in ri_set (trigdata=0x7ffe38d924c0,
is_set_null=, tgkind=)
at
../../../../../../pgsql/source/master/src/backend/utils/adt/ri_triggers.c:1219
#3 0x5610d8f897ea in ExecCallTriggerFunc
(trigdata=trigdata@entry=0x7ffe38d924c0, tgindx=tgindx@entry=2,
finfo=0x5611153f5768, finfo@entry=0x5611153f5708,
instr=instr@entry=0x0,
per_tuple_context=per_tuple_context@entry=0x561115471c30) at
../../../../../pgsql/source/master/src/backend/commands/trigger.c:2362
#4 0x5610d8f8b646 in AfterTriggerExecute (trig_tuple_slot2=0x0,
trig_tuple_slot1=0x0, per_tuple_context=0x561115471c30, instr=0x0,
finfo=0x5611153f5708,
trigdesc=0x5611153f53e8, dst_relInfo=,
src_relInfo=, relInfo=0x5611153f51d8, event=0x56111546fd4c,
estate=0x5611153f4d50)
at ../../../../../pgsql/source/master/src/backend/commands/trigger.c:4498
#5 afterTriggerInvokeEvents (events=events@entry=0x5611154292a0, firing_id=1,
estate=estate@entry=0x5611153f4d50, delete_ok=delete_ok@entry=false)
at ../../../../../pgsql/source/master/src/backend/commands/trigger.c:4735
#6 0x5610d8f8ff10 in AfterTriggerEndQuery
(estate=estate@entry=0x5611153f4d50) at
../../../../../pgsql/source/master/src/backend/commands/trigger.c:5090
#7 0x5610d8fb2457 in standard_ExecutorFinish (queryDesc=0x561115460810) at
../../../../../pgsql/source/master/src/backend/executor/execMain.c:442
#8 0x5610d917df48 in ProcessQuery (plan=0x56111545ef50,
sourceText=0x56111539cfb0 "DELETE FROM t;", params=0x0, queryEnv=0x0,
dest=0x56111545f0b0,
qc=0x7ffe38d92830) at
../../../../../pgsql/source/master/src/backend/tcop/pquery.c:193
and then
(gdb) frame 2
#2 0x5610d924a8c5 in ri_set (trigdata=0x7ffe38d924c0,
is_set_null=, tgkind=)
at
../../../../../../pgsql/source/master/src/backend/utils/adt/ri_triggers.c:1219
1219quoteOneName(attname, RIAttName(fk_rel,
set_cols[i]));
(gdb) print *riinfo
$5 = {constraint_id = 16400, valid = true, constraint_root_id = 16400,
oidHashValue = 3001019032, rootHashValue = 3001019032, conname = {
data = "pt_a_fkey", '\000' }, pk_relid = 16384, fk_relid
= 16397, confupdtype = 97 'a', confdeltype = 100 'd', ndelsetcols = 1,
confdelsetcols = {2, 0 }, confmatchtype = 115 's',
hasperiod = false, nkeys = 1, pk_attnums = {1, 0 },
fk_attnums = {1,
0 }, pf_eq_oprs = {96, 0 }, pp_eq_oprs
= {96, 0 }, ff_eq_oprs = {96, 0 },
period_contained_by_oper = 0, agged_period_contained_by_oper = 0, valid_link
= {prev = 0x5611153f4c30, next = 0x5610d96b51b0
}}
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Digital and video cameras have this adjustment and film cameras don't for the
same reason dogs and cats lick themselves: because they can." (Ken Rockwell)
Re: Fix C23 compiler warning
Alvaro Herrera writes: > On 2024-Oct-22, Peter Eisentraut wrote: >> One thing I didn't realize until today is that currently C23 compilations >> only work with meson. The autoconf version we are using doesn't support it, >> and the configure results it produces are somehow faulty and then you get a >> bunch of compilation errors. So if we wanted to make this a supported >> thing, it looks like we would need to use at least autoconf 2.72. > Oh wow. Should we at least update our autoconf requirement to 2.72 in > master? As I recall, we looked at adopting it some years ago and decided it was too much churn for the value (seeing that the long-term plan is to go to meson only). Maybe C23 is a reason to rethink, but from what I recall of that, it won't be a painless update. regards, tom lane
Improve error messages for database object stats manipulation functions during recovery
Hi,
When database object stats manipulation functions like pg_set_relation_stats()
are run,
they currently produce the following error and hint messages, which are
"internal"
and make it hard for users to understand the issue:
ERROR: cannot acquire lock mode ShareUpdateExclusiveLock on database
objects while recovery is in progress
HINT: Only RowExclusiveLock or less can be acquired on database objects
during recovery.
So I'd like to propose updating these to clearer messages:
ERROR: recovery is in progress
HINT: Database object statistics manipulation functions cannot be
executed during recovery.
Thought?
I've attached a patch implementing these changes. It also updates the
documentation to
clearly state that these functions are not available during recovery.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
From d0245c92ee068c2715c16a34450799a670b5fc7c Mon Sep 17 00:00:00 2001
From: Fujii Masao
Date: Sat, 26 Oct 2024 01:09:46 +0900
Subject: [PATCH v1] Improve error message for database object stats
manipulation functions.
Previously, database object statistics manipulation functions like
pg_set_relation_stats() reported unclear error and hint messages
when executed during recovery. These messages were "internal",
making it difficult for users to understand the issue:
ERROR: cannot acquire lock mode ShareUpdateExclusiveLock on database objects
while recovery is in progress
HINT: Only RowExclusiveLock or less can be acquired on database objects
during recovery.
This commit updates the error handling so that, if these functions
are called during recovery, they produce clearer messages:
ERROR: recovery is in progress
HINT: Database object statistics manipulation functions cannot be executed
during recovery.
The related documentation has also been updated to explicitly
clarify that these functions are not available during recovery.
---
doc/src/sgml/func.sgml | 1 +
src/backend/statistics/attribute_stats.c | 12
src/backend/statistics/relation_stats.c | 6 ++
3 files changed, 19 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7be0324ac8..1245b56305 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -30182,6 +30182,7 @@ DETAIL: Make sure pg_wal_replay_wait() isn't called
within a transaction with a
lists functions used to
manipulate statistics.
+These functions cannot be executed during recovery.
Changes made by these statistics manipulation functions are likely to be
diff --git a/src/backend/statistics/attribute_stats.c
b/src/backend/statistics/attribute_stats.c
index af61fd79e4..c6163d1cd9 100644
--- a/src/backend/statistics/attribute_stats.c
+++ b/src/backend/statistics/attribute_stats.c
@@ -155,6 +155,12 @@ attribute_statistics_update(FunctionCallInfo fcinfo, int
elevel)
stats_check_required_arg(fcinfo, attarginfo, ATTRELATION_ARG);
reloid = PG_GETARG_OID(ATTRELATION_ARG);
+ if (RecoveryInProgress())
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+errmsg("recovery is in progress"),
+errhint("Database object statistics
manipulation functions cannot be executed during recovery.")));
+
/* lock before looking up attribute */
stats_lock_check_privileges(reloid);
@@ -860,6 +866,12 @@ pg_clear_attribute_stats(PG_FUNCTION_ARGS)
stats_check_required_arg(fcinfo, attarginfo, ATTRELATION_ARG);
reloid = PG_GETARG_OID(ATTRELATION_ARG);
+ if (RecoveryInProgress())
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+errmsg("recovery is in progress"),
+errhint("Database object statistics
manipulation functions cannot be executed during recovery.")));
+
stats_lock_check_privileges(reloid);
stats_check_required_arg(fcinfo, attarginfo, ATTNAME_ARG);
diff --git a/src/backend/statistics/relation_stats.c
b/src/backend/statistics/relation_stats.c
index 5a2aabc921..48d12f88e2 100644
--- a/src/backend/statistics/relation_stats.c
+++ b/src/backend/statistics/relation_stats.c
@@ -72,6 +72,12 @@ relation_statistics_update(FunctionCallInfo fcinfo, int
elevel)
stats_check_required_arg(fcinfo, relarginfo, RELATION_ARG);
reloid = PG_GETARG_OID(RELATION_ARG);
+ if (RecoveryInProgress())
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+errmsg("recovery is in progress"),
+errhint("Database object statistics
manipulation functions cannot be executed during recovery.")));
+
s
Re: general purpose array_sort
On Fri, Oct 25, 2024 at 8:02 PM Junwang Zhao wrote: > > On Fri, Oct 25, 2024 at 1:19 AM Aleksander Alekseev > wrote: > > > > Hi, > > > > > I can accept this outcome though an optional three-valued boolean sort > > > order (ascending and descending only) I'd argue is worth keeping. null > > > value placement too I guess, three-valued boolean (nulls_first). > > > > Perhaps these optional arguments deserve separate discussions. I > > suggest merging something everyone agrees on first. This will simplify > > the review process and allow us to deliver value to the users quickly. > > Arguments like `reverse => true` and `nulls_first => true` can always > > be implemented and added as separate patches. > > As this patch uses the tuplesort infrastructure, we need to supply the > sortOperator, sortCollation and nullsFirstFlag, I tend to agree with > David. I admit that the parsing part is not good, so I will remove it > by using two boolean parameters Jian suggested earlier. > > Will send out another version by tomorrow. Based on the previous discussion, I split it into two patches in V8. 0001 is the general sort part without `is_ascending` or `nulls_first`, the sort order is determined by the "<" operator of the element type. It also cached the type entry of both eletyp and the corresponding array type. 0002 adds the `is_ascending` and `nulls_first` part, it now uses two boolean parameters instead of parsing one text parameter. > > > > > -- > > Best regards, > > Aleksander Alekseev > > > > -- > Regards > Junwang Zhao -- Regards Junwang Zhao v8-0002-support-sort-order-and-nullsfirst-flag.patch Description: Binary data v8-0001-general-purpose-array_sort.patch Description: Binary data
Re: Can rs_cindex be < 0 for bitmap heap scans?
On Fri, Oct 25, 2024 at 10:07 PM Melanie Plageman
wrote:
> On Fri, Oct 25, 2024 at 10:29 AM Melanie Plageman
> wrote:
> >
> > Tom suggested off-list that if rs_cindex can't be zero, then it should
> > be unsigned. I checked the other scan types using the
> > HeapScanDescData, and it seems none of them use values of rs_cindex or
> > rs_ntuples < 0. As such, here is a patch making both rs_ntuples and
> > rs_cindex unsigned.
>
>
@@ -943,8 +945,8 @@ heapgettup_pagemode(HeapScanDesc scan,
{
HeapTuple tuple = &(scan->rs_ctup);
Page page;
- int lineindex;
- int linesleft;
+ uint32 lineindex;
+ uint32 linesleft;
IMHO we can't make "lineindex" as uint32, because just see the first code
block[1] of heapgettup_pagemode(), we use this index as +ve (Forward scan
)as well as -ve (Backward scan).
[1]
if (likely(scan->rs_inited))
{
/* continue from previously returned page/tuple */
page = BufferGetPage(scan->rs_cbuf);
lineindex = scan->rs_cindex + dir;
if (ScanDirectionIsForward(dir))
--Refer definition of ScanDirection
typedef enum ScanDirection
{
BackwardScanDirection = -1,
NoMovementScanDirection = 0,
ForwardScanDirection = 1
} ScanDirection;
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Re: general purpose array_sort
On Thu, Oct 24, 2024 at 7:58 AM Aleksander Alekseev < aleksan...@timescale.com> wrote: > Hi, > > > It's hardly "general purpose" if it randomly refuses to > > sort certain types. I would say it should be able to sort > > anything that ORDER BY will handle --- and that certainly > > includes the cases shown here. > > I wonder how useful / convenient the new function will be considering > that we already have CTEs and can do: > > SELECT array_agg(x ORDER BY x) FROM unnest(ARRAY[5,1,3,2,4]) AS x; > > Perhaps there are use cases I didn't consider? > > Succinctness of expression. Plus I'm under the impression that a function doing this is going to be somewhat faster than composing two functions together within a multi-node subtree. I feel like the same observation could have been made for array_shuffle but we added that. This function being added feels to me like just completing the set. David J.
Docs Build in CI failing with "failed to load external entity"
Hi, I know in the past docs builds failing with "failed to load external entity" have happened on macos. But, recently I've noticed this failure for docs build on CI (not on macos) -- docs build is one of the jobs run under the "Compiler Warnings" task. See an example of this on CI for the github mirror [1]. Anyone know what the story is here? I couldn't find an existing thread on this specific issue. - Melanie [1] https://github.com/postgres/postgres/runs/32028560196
Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails
Hello Alvaro, 22.10.2024 17:32, Alvaro Herrera wrote: Yeah. I pushed these patches finally, thanks! Please look at a new anomaly introduced with 53af9491a. When running the following script: CREATE TABLE t (a int, b int, PRIMARY KEY (a, b)); CREATE TABLE pt (a int, b int, FOREIGN KEY (a, b) REFERENCES t(a, b)) PARTITION BY LIST (a); CREATE TABLE tp1 (x int, a int, b int); ALTER TABLE tp1 DROP COLUMN x; ALTER TABLE pt ATTACH PARTITION tp1 FOR VALUES IN (1); ALTER TABLE pt DETACH PARTITION tp1; I get a memory access error detected by Valgrind: 2024-10-24 12:05:04.645 UTC [1079077] LOG: statement: ALTER TABLE pt DETACH PARTITION tp1; ==00:00:00:07.887 1079077== Invalid read of size 2 ==00:00:00:07.887 1079077== at 0x4A61DD: DetachPartitionFinalize (tablecmds.c:19545) ==00:00:00:07.887 1079077== by 0x4A5C11: ATExecDetachPartition (tablecmds.c:19386) ==00:00:00:07.887 1079077== by 0x48561E: ATExecCmd (tablecmds.c:5540) ==00:00:00:07.887 1079077== by 0x4845DE: ATRewriteCatalogs (tablecmds.c:5203) ==00:00:00:07.887 1079077== by 0x4838EC: ATController (tablecmds.c:4758) ==00:00:00:07.887 1079077== by 0x4834F1: AlterTable (tablecmds.c:4404) ==00:00:00:07.887 1079077== by 0x7D6D52: ProcessUtilitySlow (utility.c:1318) ==00:00:00:07.887 1079077== by 0x7D65F7: standard_ProcessUtility (utility.c:1067) ==00:00:00:07.887 1079077== by 0x7D54F7: ProcessUtility (utility.c:523) ==00:00:00:07.887 1079077== by 0x7D3D70: PortalRunUtility (pquery.c:1158) ==00:00:00:07.887 1079077== by 0x7D3FE7: PortalRunMulti (pquery.c:1316) ==00:00:00:07.887 1079077== by 0x7D3431: PortalRun (pquery.c:791) Reproduced on REL_15_STABLE .. master. Best regards, Alexander
Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails
On 2024-Oct-25, Tender Wang wrote: > Thanks for reporting. I can reproduce this memory invalid access on HEAD. > After debuging codes, I found the root cause. > In DetachPartitionFinalize(), below code: > att = TupleDescAttr(RelationGetDescr(partRel), >attmap->attnums[conkey[i] - 1] - 1); > > We should use confkey[i] -1 not conkey[i] - 1; Wow, how embarrasing, now that's one _really_ stupid bug and it's totally my own. Thanks for the analysis! I'll get this patched soon. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: type cache cleanup improvements
Hi, On 2024-10-22 20:33:24 +0300, Alexander Korotkov wrote: > Thank you, Pavel! 0001 revised according to your suggestion. Starting with this commit CI fails. https://cirrus-ci.com/task/6668851469877248 https://api.cirrus-ci.com/v1/artifact/task/6668851469877248/testrun/build/testrun/regress-running/regress/regression.diffs diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out --- /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out 2024-10-24 11:38:43.829712000 + +++ /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out 2024-10-24 11:44:57.154238000 + @@ -1338,14 +1338,9 @@ ERROR: cannot drop inherited constraint "f1_pos" of relation "p1_c1" alter table p1 drop constraint f1_pos; \d p1_c1 - Table "public.p1_c1" - Column | Type | Collation | Nullable | Default -+-+---+--+- - f1 | integer | | | -Check constraints: -"f1_pos" CHECK (f1 > 0) -Inherits: p1 - +ERROR: error triggered for injection point typecache-before-rel-type-cache-insert +LINE 4: ORDER BY 1; + ^ drop table p1 cascade; NOTICE: drop cascades to table p1_c1 create table p1(f1 int constraint f1_pos CHECK (f1 > 0)); Greetings, Andres
Re: type cache cleanup improvements
On Fri, Oct 25, 2024 at 12:48 PM Alexander Korotkov wrote: > On Fri, Oct 25, 2024 at 11:35 AM Andres Freund wrote: > > On 2024-10-22 20:33:24 +0300, Alexander Korotkov wrote: > > > Thank you, Pavel! 0001 revised according to your suggestion. > > > > Starting with this commit CI fails. > > > > https://cirrus-ci.com/task/6668851469877248 > > https://api.cirrus-ci.com/v1/artifact/task/6668851469877248/testrun/build/testrun/regress-running/regress/regression.diffs > > > > diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out > > /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out > > --- /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out 2024-10-24 > > 11:38:43.829712000 + > > +++ > > /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out > > 2024-10-24 11:44:57.154238000 + > > @@ -1338,14 +1338,9 @@ > > ERROR: cannot drop inherited constraint "f1_pos" of relation "p1_c1" > > alter table p1 drop constraint f1_pos; > > \d p1_c1 > > - Table "public.p1_c1" > > - Column | Type | Collation | Nullable | Default > > -+-+---+--+- > > - f1 | integer | | | > > -Check constraints: > > -"f1_pos" CHECK (f1 > 0) > > -Inherits: p1 > > - > > +ERROR: error triggered for injection point > > typecache-before-rel-type-cache-insert > > +LINE 4: ORDER BY 1; > > + ^ > > drop table p1 cascade; > > NOTICE: drop cascades to table p1_c1 > > create table p1(f1 int constraint f1_pos CHECK (f1 > 0)); > > Thank you for reporting this. > Looks weird that injection point, which isn't used in these tests, got > triggered here. > I'm looking into this. Oh, I forgot to make injection points in typcache_rel_type_cache.sql local. Thus, it affects concurrent tests. Must be fixed in aa1e898dea. -- Regards, Alexander Korotkov Supabase
Re: Conflict detection for update_deleted in logical replication
Hello, Hayato! > Thanks for updating the patch! While reviewing yours, I found a corner case that > a recently deleted tuple cannot be detected when index scan is chosen. > This can happen when indices are re-built during the replication. > Unfortunately, I don't have any solutions for it. I just randomly saw your message, so, I could be wrong and out of the context - so, sorry in advance. But as far as I know, to solve this problem, we need to wait for slot.xmin during the [0] (WaitForOlderSnapshots) while creating index concurrently. [1]: https://github.com/postgres/postgres/blob/68dfecbef210dc000271553cfcb2342989d4ca0f/src/backend/commands/indexcmds.c#L1758-L1765 Best regards, Mikhail.
Re: Pgoutput not capturing the generated columns
On Fri, Oct 25, 2024 at 12:07 PM Amit Kapila wrote: > > On Thu, Oct 24, 2024 at 8:50 PM vignesh C wrote: > > > > The v42 version patch attached at [1] has the changes for the same. > > > > Some more comments: > 1. +pgoutput_pubgencol_init(PGOutputData *data, List *publications, + RelationSyncEntry *entry) Can we name it as check_and_init_gencol? I don't know if it is a good idea to append a prefix pgoutput for local functions. It is primarily used for exposed functions from pgoutput.c. I see that in a few cases we do that for local functions as well but that is not a norm. A related point: + /* Initialize publish generated columns value */ + pgoutput_pubgencol_init(data, rel_publications, entry); Accordingly change this comment to something like: "Check whether to publish to generated columns.". 2. +/* + * Returns true if the relation has column list associated with the + * publication, false if the relation has no column list associated with the + * publication. + */ +bool +is_column_list_publication(Publication *pub, Oid relid) ... ... How about naming the above function as has_column_list_defined()? Also, you can write the above comment as: "Returns true if the relation has column list associated with the publication, false otherwise." 3. + /* + * The column list takes precedence over pubgencols, so skip checking + * column list publications. + */ + if (is_column_list_publication(pub, entry->publish_as_relid)) Let's change this comment to: "The column list takes precedence over publish_generated_columns option. Those will be checked later, see pgoutput_column_list_init." -- With Regards, Amit Kapila.
Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails
Hello Alvaro and Tender Wang, 24.10.2024 17:00, Alexander Lakhin wrote: Please look at a new anomaly introduced with 53af9491a. When running the following script: CREATE TABLE t (a int, b int, PRIMARY KEY (a, b)); CREATE TABLE pt (a int, b int, FOREIGN KEY (a, b) REFERENCES t(a, b)) PARTITION BY LIST (a); CREATE TABLE tp1 (x int, a int, b int); ALTER TABLE tp1 DROP COLUMN x; ALTER TABLE pt ATTACH PARTITION tp1 FOR VALUES IN (1); ALTER TABLE pt DETACH PARTITION tp1; I've also discovered another anomaly with a similar setup, but it's not related to 53af9491a. CREATE TABLE t (a int, PRIMARY KEY (a)); INSERT INTO t VALUES (1); CREATE TABLE pt (b int, a int) PARTITION BY RANGE (a); ALTER TABLE pt DROP COLUMN b; ALTER TABLE pt ADD FOREIGN KEY (a) REFERENCES t ON DELETE SET DEFAULT (a); CREATE TABLE tp1 PARTITION OF pt FOR VALUES FROM (1) TO (2); ALTER TABLE pt DETACH PARTITION tp1; DELETE FROM t; \d+ t This script ends up with: ERROR: invalid attribute number 2 ERROR: cache lookup failed for attribute 2 of relation 16398 (Perhaps it deserves a separate discussion.) Best regards, Alexander
Re: Forbid to DROP temp tables of other sessions
On Fri, 25 Oct 2024 at 11:02, Daniil Davydov <3daniss...@gmail.com> wrote: > But are there any reasons to allow us to DROP such tables? > Hi! This topic has already been discussed in [0], I believe. I'm not sure how it all ended and if there were any changes made in the end. But from the user's perspective, temporary tables are expected to be isolated within sessions, I think. This is an ideal solution, but does it feasible or not is a question. BTW, if we can "isolate" temp relations, we'll be one step close to get rid of temp relations locking [1]. [0] https://www.postgresql.org/message-id/flat/d4a68c6d-d6c4-d52a-56cb-babb8177b5fe%40oss.nttdata.com [1] https://www.postgresql.org/message-id/flat/CACG%3DezZzMbjMzshhe%2BLDd4NJ0SeRPvCH9%2BLFS7SAPbM6Qxwe5g%40mail.gmail.com -- Best regards, Maxim Orlov.
Re: Alias of VALUES RTE in explain plan
Hi Ashutosh & PG Hackers,
I have fixed the code to produce desired output by adding a few lines in
pull_up_simple_subquery().
Attached patch is divided in 2 files:
- 001-Fix-Alias-VALUES-RTE.patch contains the actual fix.
- 002-Fix-Alias-VALUES-RTE.patch contains the expected output changes
against the actual fix.
I also have verified regression tests, all seems good.
Respected hackers please have a look.
Thanks and regards...
Yasir
On Thu, Aug 15, 2024 at 7:13 PM Yasir wrote:
>
>
> On Mon, Jul 1, 2024 at 3:17 PM Ashutosh Bapat <
> ashutosh.bapat@gmail.com> wrote:
>
>> Hi All,
>> While reviewing Richard's patch for grouping sets, I stumbled upon
>> following explain output
>>
>> explain (costs off)
>> select distinct on (a, b) a, b
>> from (values (1, 1), (2, 2)) as t (a, b) where a = b
>> group by grouping sets((a, b), (a))
>> order by a, b;
>>QUERY PLAN
>>
>> Unique
>>-> Sort
>> Sort Key: "*VALUES*".column1, "*VALUES*".column2
>> -> HashAggregate
>>Hash Key: "*VALUES*".column1, "*VALUES*".column2
>>Hash Key: "*VALUES*".column1
>>-> Values Scan on "*VALUES*"
>> Filter: (column1 = column2)
>> (8 rows)
>>
>> There is no VALUES.column1 and VALUES.column2 in the query. The alias t.a
>> and t.b do not appear anywhere in the explain output. I think explain
>> output should look like
>> explain (costs off)
>> select distinct on (a, b) a, b
>> from (values (1, 1), (2, 2)) as t (a, b) where a = b
>> group by grouping sets((a, b), (a))
>> order by a, b;
>>QUERY PLAN
>>
>> Unique
>>-> Sort
>> Sort Key: t.a, t.b
>> -> HashAggregate
>>Hash Key: t.a, t.b
>>Hash Key: t.a
>>-> Values Scan on "*VALUES*" t
>> Filter: (a = b)
>> (8 rows)
>>
>> I didn't get time to figure out the reason behind this, nor the history.
>> But I thought I would report it nonetheless.
>>
>
> I have looked into the issue and found that when subqueries are pulled up,
> a modifiable copy of the subquery is created for modification in the
> pull_up_simple_subquery() function. During this process,
> flatten_join_alias_vars() is called to flatten any join alias variables
> in the subquery's target list. However at this point, we lose
> subquery's alias.
> If you/hackers agree with my findings, I can provide a working patch soon.
>
>
>> --
>> Best Wishes,
>> Ashutosh Bapat
>>
>
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 5482ab85a7..e751ae21d1 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -98,7 +98,8 @@ static bool is_simple_subquery(PlannerInfo *root, Query *subquery,
JoinExpr *lowest_outer_join);
static Node *pull_up_simple_values(PlannerInfo *root, Node *jtnode,
RangeTblEntry *rte);
-static bool is_simple_values(PlannerInfo *root, RangeTblEntry *rte);
+static bool is_simple_values(PlannerInfo *root, RangeTblEntry *rte,
+ bool allow_multi_values);
static Node *pull_up_constant_function(PlannerInfo *root, Node *jtnode,
RangeTblEntry *rte,
AppendRelInfo *containing_appendrel);
@@ -910,7 +911,7 @@ pull_up_subqueries_recurse(PlannerInfo *root, Node *jtnode,
if (rte->rtekind == RTE_VALUES &&
lowest_outer_join == NULL &&
containing_appendrel == NULL &&
- is_simple_values(root, rte))
+ is_simple_values(root, rte, false))
return pull_up_simple_values(root, jtnode, rte);
/*
@@ -990,6 +991,33 @@ pull_up_subqueries_recurse(PlannerInfo *root, Node *jtnode,
return jtnode;
}
+static RangeTblEntry *get_rte_from_values_query(PlannerInfo *root, Query *subquery)
+{
+ RangeTblRef *rtr = NULL;
+ RangeTblEntry *rte = NULL;
+ Node *node;
+
+ if (subquery->jointree == NULL ||
+ list_length(subquery->jointree->fromlist) != 1)
+ return NULL;
+
+ if (list_length(subquery->rtable) != 1)
+ return NULL;
+
+ node = linitial(subquery->jointree->fromlist);
+ if (!IsA(node, RangeTblRef))
+ return NULL;
+
+ rtr = castNode(RangeTblRef, node);
+ rte = rt_fetch(rtr->rtindex, subquery->rtable);
+
+ /* elog_node_display(LOG, "YH | rte tree", root->parse, true); */
+ if (rte == NULL || rte->rtekind != RTE_VALUES)
+ return NULL;
+
+ return is_simple_values(root, rte, true) ? rte : NULL;
+}
+
/*
* pull_up_simple_subquery
* Attempt to pull up a single simple subquery.
@@ -1014,6 +1042,7 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
int rtoffset;
pullup_replace_vars_context rvcontext;
ListCell *lc;
+ RangeTblEntry *values_rte = NULL;
/*
* Make a modifiable copy of the subquery to hack on, so that the RTE will
@@ -1124,6 +1153,12 @@ pull_up_
Re: Can rs_cindex be < 0 for bitmap heap scans?
On Thu, Oct 24, 2024 at 9:40 AM Melanie Plageman
wrote:
>
> On Thu, Oct 24, 2024 at 12:50 AM Dilip Kumar wrote:
> >
> > On Thu, Oct 24, 2024 at 3:45 AM Melanie Plageman
> > wrote:
> >>
> >> HeapScanDescData->rs_cindex (the current index into the array of
> >> visible tuples stored in the heap scan descriptor) is used for
> >> multiple scan types, but for bitmap heap scans, AFAICT, it should
> >> never be < 0.
> >
> > You are right it can never be < 0 in this case at least.
>
> Cool, thanks for confirming. I will commit this change then.
Tom suggested off-list that if rs_cindex can't be zero, then it should
be unsigned. I checked the other scan types using the
HeapScanDescData, and it seems none of them use values of rs_cindex or
rs_ntuples < 0. As such, here is a patch making both rs_ntuples and
rs_cindex unsigned.
- Melanie
From b770f675c8130bfa2d1a25fbfc8064f1dd7cbbe6 Mon Sep 17 00:00:00 2001
From: Melanie Plageman
Date: Fri, 25 Oct 2024 09:09:24 -0400
Subject: [PATCH v2] Make rs_cindex and rs_ntuples unsigned
HeapScanDescData.rs_cindex and rs_ntuples can't be less than 0. All scan
types using the heap scan descriptor expect these values to be >= 0.
Make that expectation clear by making rs_cindex and rs_ntuples unsigned.
Also remove the test in heapam_scan_bitmap_next_tuple() that checks if
rs_cindex < 0. This was never true, but now that rs_cindex is unsigned,
it makes even less sense.
While we are at it, initialize both rs_cindex and rs_ntuples to 0 in
initscan().
Author: Melanie Plageman
---
src/backend/access/heap/heapam.c | 7 +--
src/backend/access/heap/heapam_handler.c | 2 +-
src/include/access/heapam.h | 4 ++--
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 4c8febdc811..d127b75a1a5 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -378,6 +378,8 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
ItemPointerSetInvalid(&scan->rs_ctup.t_self);
scan->rs_cbuf = InvalidBuffer;
scan->rs_cblock = InvalidBlockNumber;
+ scan->rs_ntuples = 0;
+ scan->rs_cindex = 0;
/*
* Initialize to ForwardScanDirection because it is most common and
@@ -943,8 +945,8 @@ heapgettup_pagemode(HeapScanDesc scan,
{
HeapTuple tuple = &(scan->rs_ctup);
Page page;
- int lineindex;
- int linesleft;
+ uint32 lineindex;
+ uint32 linesleft;
if (likely(scan->rs_inited))
{
@@ -989,6 +991,7 @@ continue_page:
ItemId lpp;
OffsetNumber lineoff;
+ Assert(lineindex >= 0 && lineindex <= scan->rs_ntuples);
lineoff = scan->rs_vistuples[lineindex];
lpp = PageGetItemId(page, lineoff);
Assert(ItemIdIsNormal(lpp));
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 8c59b77b64f..82668fab19a 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2269,7 +2269,7 @@ heapam_scan_bitmap_next_tuple(TableScanDesc scan,
/*
* Out of range? If so, nothing more to look at on this page
*/
- if (hscan->rs_cindex < 0 || hscan->rs_cindex >= hscan->rs_ntuples)
+ if (hscan->rs_cindex >= hscan->rs_ntuples)
return false;
targoffset = hscan->rs_vistuples[hscan->rs_cindex];
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index b951466ced2..21c7b70edf2 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -103,8 +103,8 @@ typedef struct HeapScanDescData
int rs_empty_tuples_pending;
/* these fields only used in page-at-a-time mode and for bitmap scans */
- int rs_cindex; /* current tuple's index in vistuples */
- int rs_ntuples; /* number of visible tuples on page */
+ uint32 rs_cindex; /* current tuple's index in vistuples */
+ uint32 rs_ntuples; /* number of visible tuples on page */
OffsetNumber rs_vistuples[MaxHeapTuplesPerPage]; /* their offsets */
} HeapScanDescData;
typedef struct HeapScanDescData *HeapScanDesc;
--
2.34.1
Re: processes stuck in shutdown following OOM/recovery
At commit 779972e, I got about 50% "pg_ctl: server does not shut down" from
$SUBJECT with this loop:
nti=; while date; do PGCTLTIMEOUT=4 make check-tests TESTS=reindex_catalog
PG_TEST_INITDB_EXTRA_OPTS='-cwal_level=minimal -cmax_wal_senders=0'
NO_TEMP_INSTALL=$nti; nti=1; grep abnormal src/test/regress/log/postmaster.log;
done
Your patch fixes that. (It came to my attention while developing commit
67bab53. At that commit or later, this recipe won't see the problem.) The
patch is a strict improvement, so I would be marking
https://commitfest.postgresql.org/50/4884/ Ready for Committer if it weren't
already in that state. That said, ...
On Thu, May 23, 2024 at 10:29:13AM +1200, Thomas Munro wrote:
> I'm also hoping to get review of the rather finickity state machine
> logic involved from people familiar with that; I think it's right, but
> I'd hate to break some other edge case...
... while I don't see an edge case it makes worse, I would fix the defect
differently, to cover more existing edge cases. I think a good standard is to
align with restart_after_crash=off behaviors. With restart_after_crash=off,
the postmaster exits with "shutting down because \"restart_after_crash\" is
off". Assume an external service manager then restarts it. Any deviations
between that experience and running with restart_after_crash=on should have a
specific reason. So post-OOM recovery should mirror original crash recovery
of a fresh postmaster.
List of behaviors the postmaster FatalError variable controls:
arms "issuing %s to recalcitrant children"
disarms "terminating any other active server processes"
changes CAC_STARTUP to CAC_RECOVERY
makes PM_WAIT_BACKENDS assume checkpointer already got SIGQUIT [causes
$SUBJECT hang]
exit(1), as opposed to exit(0)
LOG "abnormal database system shutdown"
"all server processes terminated; reinitializing"
disarm maybe_start_bgworkers
Once we launch a new startup process for post-OOM recovery, I think we want
all of those behaviors to cease. In particular, disarming
maybe_start_bgworkers() and disarming "terminating any other active server
processes" are bad at that point. (And the $SUBJECT hang is bad, of course.)
What do you think? The rest are more cosmetic. (We don't document the exit
status, and pg_ctl ignores it. CAC_STARTUP gives me the most ambivalence, but
its cosmetics don't matter too much.) Disabling all those entails essentially
the following, though it would deserve comment edits and removal of the later
FatalError=false:
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -3144,8 +3144,9 @@ PostmasterStateMachine(void)
StartupPID = StartChildProcess(B_STARTUP);
Assert(StartupPID != 0);
StartupStatus = STARTUP_RUNNING;
SetPmState(PM_STARTUP);
+ FatalError = false;
/* crash recovery started, reset SIGKILL flag */
AbortStartTime = 0;
Your patch eliminate all hangs for me, but about half the iterations of the
above test command still get exit(1) and the "abnormal" message. That could
be fine. Perhaps that matches what would happen if the fast shutdown arrived
earlier, after the postmaster observed the crash and before the last backend
reacted to SIGQUIT. Still, I'd lean toward clearing FatalError once we launch
the startup process. Again, two other behaviors it controls feel bad at that
point, and none of the behaviors it controls feel clearly-good at that point.
> --- a/src/backend/postmaster/postmaster.c
> +++ b/src/backend/postmaster/postmaster.c
> @@ -3748,12 +3748,14 @@ PostmasterStateMachine(void)
> WalSummarizerPID == 0 &&
> BgWriterPID == 0 &&
> (CheckpointerPID == 0 ||
> - (!FatalError && Shutdown < ImmediateShutdown)) &&
> + (!FatalError && Shutdown < ImmediateShutdown) ||
> + (FatalError && CheckpointerPID != 0)) &&
I think that's equivalent to:
(CheckpointerPID == 0 || FatalError || Shutdown < ImmediateShutdown)
Separable topic: this related comment and code are already out of date:
/*
* Unexpected exit of startup process (including FATAL
exit)
* during PM_STARTUP is treated as catastrophic. There
are no
* other processes running yet, so we can just exit.
*/
if (pmState == PM_STARTUP &&
StartupStatus != STARTUP_SIGNALED &&
!EXIT_STATUS_0(exitstatus))
{
LogChildExit(LOG, _("startup process"),
pid, exitstatus);
ereport(LOG,
(errmsg("aborting
Re: Fix for consume_xids advancing XIDs incorrectly
On Thu, Oct 24, 2024 at 7:55 AM Fujii Masao wrote:
>
>
>
> On 2024/10/24 5:23, Masahiko Sawada wrote:
> >> if (xids_left > 2000 &&
> >> consumed - last_reported_at < REPORT_INTERVAL &&
> >> MyProc->subxidStatus.overflowed)
> >> {
> >> int64 consumed_by_shortcut =
> >> consume_xids_shortcut();
> >>
> >> if (consumed_by_shortcut > 0)
> >> {
> >> consumed += consumed_by_shortcut;
> >> continue;
> >> }
> >> }
> >>
> >> By the way, this isn't directly related to the proposed patch, but while
> >> reading
> >> the xid_wraparound code, I noticed that consume_xids_common() could
> >> potentially
> >> return an unexpected XID if consume_xids_shortcut() returns a value greater
> >> than 2000. Based on the current logic, consume_xids_common() should always
> >> return
> >> a value less than 2000, so the issue I'm concerned about shouldn't occur.
> >
> > Good point. Yes, the function doesn't return a value greater than 2000
> > as long as we do "distance = Min(distance, COMMIT_TS_XACTS_PER_PAGE -
> > rem);". But it's true with <= 16KB block sizes.
> >
> >> Still,
> >> would it be worth adding an assertion to ensure that consume_xids_common()
> >> never
> >> returns a value greater than 2000?
> >
> > While adding an assertion makes sense to me, another idea is to set
> > last_xid even in the shortcut path. That way, it works even with 32KB
> > block size.
>
> Agreed on making xid_wraparound compatible with a 32k block size. As you
> pointed out,
> with 32k blocks, XidSkip() can return values greater than 2000. So, the first
> step is
> to adjust the following if-condition by increasing "2000" to a higher value.
> Since XidSkip() can return up to 3276 (based on COMMIT_TS_XACTS_PER_PAGE
> (BLCKSZ / 10) with 32k blocks),
> we could, for instance, update "2000" to "4000."
>
> if (xids_left > 2000 &&
> consumed - last_reported_at < REPORT_INTERVAL &&
> MyProc->subxidStatus.overflowed)
>
>
> To ensure lastxid is set even in the shortcut case, what about modifying
> XidSkip()
> so it returns "distance - 1" instead of "distance"? This change would make
> consume_xids_common() run at least one more loop cycle,
> triggering GetNewTransactionId() and setting lastxid correctly.
Increasing "2000" to "4000" makes sense to me. I think that with this
change we don't necessarily need to change XidSkip() to return
"distance - 1'. What do you think?
>
>
> consumed = XidSkip(nextXid);
> if (consumed > 0)
> TransamVariables->nextXid.value += (uint64) consumed;
>
> BTW, the code snippet above in consume_xids_shortcut() could potentially set
> the next XID to a value below FirstNormalTransactionId? If yes, we should
> account for
> FirstNormalFullTransactionId when increasing the next XID, similar to
> how FullTransactionIdAdvance() handles it.
Good catch. I agree with you. We need to do something similar to what
FullTransactionIdAdvance() does so that it does not appear as a
special 32-bit XID.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Re: cpluspluscheck complains about use of register
On Fri Oct 25, 2024 at 3:04 PM CDT, Tom Lane wrote: Christoph Berg writes: Should the removal of "register" be backported to support that better? Perhaps. It's early days yet, but nobody has complained that that broke anything in v16, so I'm guessing it'd be fine. FWIW, I ran into this compiling an extension written in C++ for v15, so I'll throw my support for backpatching this if that is still on the table. Understandable if not, though. -- Tristan Partin Neon (https://neon.tech)
Re: Docs Build in CI failing with "failed to load external entity"
Hi, On 2024-10-25 04:14:03 -0400, Andres Freund wrote: > On 2024-10-25 08:22:42 +0300, Thomas Munro wrote: > > I wonder if this will magically fix itself when the next CI image > > build cron job kicks off. I have no idea what time zone this page is > > showing but it should happen in another day or so, unless Andres is > > around to kick it sooner: > > > > https://cirrus-ci.com/github/anarazel/pg-vm-images > > I did trigger a rebuild of the image just now. Hopefully that'll fix it. It did. Greetings, Andres Freund
Re: general purpose array_sort
On Thu, Oct 24, 2024 at 8:27 AM Aleksander Alekseev < aleksan...@timescale.com> wrote: > > Just making an observation / thinking out loud that the requirement to > support everything ORDER BY handles / supports (and will handle / > support?) might make this function impractical to maintain. > > Particularly, does the function really need to support dir => asc | > desc | asc nulls first | etc... ? Maybe array_sort() + array_reverse( > array_sort() ) will handle most of the practical cases? I don't know. > > [1]: https://commitfest.postgresql.org/50/5314/ > > Composing function calls here seems quite undesirable from a performance standpoint, but maybe I over-estimate the cost of exploding-manipulating-freezing an array datum. Also, while I'm not in a good position to judge the challenge of implementing the sort parameters I would rather have them than not since the order by API has them (plus performance). I also, maybe unreasonably, believe that our extensible type system has already limited the maintenance burden. David J.
Re: type cache cleanup improvements
On Fri, Oct 25, 2024 at 11:35 AM Andres Freund wrote: > On 2024-10-22 20:33:24 +0300, Alexander Korotkov wrote: > > Thank you, Pavel! 0001 revised according to your suggestion. > > Starting with this commit CI fails. > > https://cirrus-ci.com/task/6668851469877248 > https://api.cirrus-ci.com/v1/artifact/task/6668851469877248/testrun/build/testrun/regress-running/regress/regression.diffs > > diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out > /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out > --- /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out 2024-10-24 > 11:38:43.829712000 + > +++ > /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out > 2024-10-24 11:44:57.154238000 + > @@ -1338,14 +1338,9 @@ > ERROR: cannot drop inherited constraint "f1_pos" of relation "p1_c1" > alter table p1 drop constraint f1_pos; > \d p1_c1 > - Table "public.p1_c1" > - Column | Type | Collation | Nullable | Default > -+-+---+--+- > - f1 | integer | | | > -Check constraints: > -"f1_pos" CHECK (f1 > 0) > -Inherits: p1 > - > +ERROR: error triggered for injection point > typecache-before-rel-type-cache-insert > +LINE 4: ORDER BY 1; > + ^ > drop table p1 cascade; > NOTICE: drop cascades to table p1_c1 > create table p1(f1 int constraint f1_pos CHECK (f1 > 0)); Thank you for reporting this. Looks weird that injection point, which isn't used in these tests, got triggered here. I'm looking into this. -- Regards, Alexander Korotkov Supabase
Re: Conflict detection for update_deleted in logical replication
> Here is the V5 patch set which addressed above comments.
>
Here are a couple of comments on v5 patch-set -
1) In FindMostRecentlyDeletedTupleInfo(),
+ /* Try to find the tuple */
+ while (index_getnext_slot(scan, ForwardScanDirection, scanslot))
+ {
+ Assert(tuples_equal(scanslot, searchslot, eq));
+ update_recent_dead_tuple_info(scanslot, oldestXmin, delete_xid,
+ delete_time, delete_origin);
+ }
In my tests, I found that the above assert() triggers during
unidirectional replication of an update on a table. While doing the
replica identity index scan, it can only ensure to match the indexed
columns value, but the current Assert() assumes all the column values
should match, which seems wrong.
2) Since update_deleted requires both 'track_commit_timestamp' and the
'detect_update_deleted' to be enabled, should we raise an error in the
CREATE and ALTER subscription commands when track_commit_timestamp=OFF
but the user specifies detect_update_deleted=true?
Re: general purpose array_sort
On Fri, Oct 25, 2024 at 1:19 AM Aleksander Alekseev wrote: > > Hi, > > > I can accept this outcome though an optional three-valued boolean sort > > order (ascending and descending only) I'd argue is worth keeping. null > > value placement too I guess, three-valued boolean (nulls_first). > > Perhaps these optional arguments deserve separate discussions. I > suggest merging something everyone agrees on first. This will simplify > the review process and allow us to deliver value to the users quickly. > Arguments like `reverse => true` and `nulls_first => true` can always > be implemented and added as separate patches. As this patch uses the tuplesort infrastructure, we need to supply the sortOperator, sortCollation and nullsFirstFlag, I tend to agree with David. I admit that the parsing part is not good, so I will remove it by using two boolean parameters Jian suggested earlier. Will send out another version by tomorrow. > > -- > Best regards, > Aleksander Alekseev -- Regards Junwang Zhao
Re: Using read_stream in index vacuum
> On 25 Oct 2024, at 00:55, Rahila Syed wrote: > > While writing this email, I realized I evicted buffers for the table > and not the index. I will perform the test again. However, > I would like to know your opinion on whether this looks like > a valid test. Well, yes, kind of, you need drop caches from index. And, perhaps, you can have more indexes. You also can disable autovaccum and just restart postgres instead of iterating through buffer caches. I've asked Thomas about performance implications and he told me that converting stuff to streamlined API is not expected to have better performance. It's needed to have decent perfromance when DIRECT_IO will be involved. Thanks! Best regards, Andrey Borodin.
Re: pg_upgrade check for invalid databases
> On 14 Oct 2024, at 18:57, Bruce Momjian wrote: > What might be acceptable would be to add an option that would make > pg_upgrade more tolerant of problems in many areas, that is a lot more > research and discussion. I agree that the concept of having pg_upgrade perform (opt-in) skipping and/or repairs of the old cluster warrants a larger discussion in its own thread. There has been significant amount of energy spent recently to add structure to the checks, any new feature should be properly designed for the get-go. In the meantime, the OP has a good point that it's a tad silly that pg_upgrade fails hard on invalid databases instead of detecting and reporting like how other errors are handled. The attached adds this check and expands the report wording to cover it. -- Daniel Gustafsson 0001-Find-invalid-databases-during-upgrade-check-stage.patch Description: Binary data
Re: Alias of VALUES RTE in explain plan
Yasir writes: > I have fixed the code to produce desired output by adding a few lines in > pull_up_simple_subquery(). > Attached patch is divided in 2 files: > - 001-Fix-Alias-VALUES-RTE.patch contains the actual fix. > - 002-Fix-Alias-VALUES-RTE.patch contains the expected output changes > against the actual fix. I was initially skeptical about this, because we've been printing "*VALUES*" for a decade or more and there have been few complaints. So I wondered if the change would annoy more people than liked it. However, after looking at the output for awhile, it is nice that the columns of the VALUES are referenced with their user-given names instead of "columnN". I think that's enough of an improvement that it'll win people over. However ... I don't like this implementation, not even a little bit. Table/column aliases are assigned by the parser, and the planner has no business overriding them. Quite aside from being a violation of system structural principles, there are practical reasons not to do it like that: 1. We'd see different results when considering plan trees than unplanned query trees. 2. At the place where you put this, some planning transformations have already been done, and that affects the results. That means that future extensions or restructuring of the planner might change the results, which seems undesirable. I think the right way to make this happen is for the parser to do it, which it can do by passing down the outer query level's Alias to addRangeTableEntryForValues. There's a few layers of subroutine calls between, but we can minimize the pain by adding a ParseState field to carry the Alias. See attached. My point 2 is illustrated by the fact that my patch produces different results in a few cases than yours does --- look at groupingsets.out in particular. I think that's fine, and the changes that yours makes and mine doesn't look a little unprincipled. For example, in the tests involving the "gstest1" view, if somebody wants nice labels on that view's VALUES columns then the right place to apply those labels is within the view. Letting a subsequent call of the view inject labels seems pretty action-at-a-distance-y. regards, tom lane diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index f2bcd6aa98..73dd1d80c8 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -8992,16 +8992,16 @@ insert into utrtest values (2, 'qux'); -- with a non-direct modification plan explain (verbose, costs off) update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x returning *; - QUERY PLAN - +QUERY PLAN +--- Update on public.utrtest - Output: utrtest_1.a, utrtest_1.b, "*VALUES*".column1 + Output: utrtest_1.a, utrtest_1.b, s.x Foreign Update on public.remp utrtest_1 Remote SQL: UPDATE public.loct SET a = $2 WHERE ctid = $1 RETURNING a, b Update on public.locp utrtest_2 -> Hash Join - Output: 1, "*VALUES*".*, "*VALUES*".column1, utrtest.tableoid, utrtest.ctid, utrtest.* - Hash Cond: (utrtest.a = "*VALUES*".column1) + Output: 1, s.*, s.x, utrtest.tableoid, utrtest.ctid, utrtest.* + Hash Cond: (utrtest.a = s.x) -> Append -> Foreign Scan on public.remp utrtest_1 Output: utrtest_1.a, utrtest_1.tableoid, utrtest_1.ctid, utrtest_1.* @@ -9009,9 +9009,9 @@ update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x returning *; -> Seq Scan on public.locp utrtest_2 Output: utrtest_2.a, utrtest_2.tableoid, utrtest_2.ctid, NULL::record -> Hash - Output: "*VALUES*".*, "*VALUES*".column1 - -> Values Scan on "*VALUES*" - Output: "*VALUES*".*, "*VALUES*".column1 + Output: s.*, s.x + -> Values Scan on s + Output: s.*, s.x (18 rows) update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x returning *; @@ -9049,16 +9049,16 @@ ERROR: cannot route tuples into foreign table to be updated "remp" -- with a non-direct modification plan explain (verbose, costs off) update utrtest set a = 3 from (values (2), (3)) s(x) where a = s.x returning *; - QUERY PLAN -- +QUERY PLAN
Re: CREATE INDEX CONCURRENTLY on partitioned index
I noticed that development on the concurrent index creation for partitioned tables feature seemed to stall a few months ago. The patch looked solid, and there didn’t seem to be any issues with it. Has there been any further progress? This feature would be invaluable, given the prevalence of partitioned tables in modern databases. Thanks for any updates! On Fri, 25 Oct 2024 at 9:14 PM, Michael Paquier wrote: > On Fri, Jul 12, 2024 at 11:17:25PM +0100, Ilya Gladyshev wrote: > > Sure, created a separate thread [1]. Please disregard the second patch in > > this thread. Duplicating the last version of the relevant patch here to > > avoid any confusion. > > > > [1] > https://www.postgresql.org/message-id/b72f2d89-820a-4fa2-9058-b155cf646f4f%40gmail.com > > Thanks, will check that. > -- > Michael >
Re: Forbid to DROP temp tables of other sessions
Daniil Davydov <3daniss...@gmail.com> writes: > I noticed that TRUNCATE and ALTER commands on temporary tables of other > sessions produce an error "cannot truncate/alter temporary tables of other > sessions". But are there any reasons to allow us to DROP such tables? > It seems to me that the only case when we may need it is the removal of > orphan tables. But the autovacuum is responsible for this and it uses a > different functionality. I'm wondering if there are any other cases. If > not, can we just handle it for example in ExecDropStmt and produce an error > like "cannot drop temporary tables of other sessions"? If autovacuum can do it, I don't see a reason to prevent superusers from doing it manually. regards, tom lane
Re: Pgoutput not capturing the generated columns
On Fri, Oct 25, 2024 at 3:54 PM Amit Kapila wrote: > > On Fri, Oct 25, 2024 at 12:07 PM Amit Kapila wrote: > > > > On Thu, Oct 24, 2024 at 8:50 PM vignesh C wrote: > > > > > > The v42 version patch attached at [1] has the changes for the same. > > > > > > > Some more comments: > > > > 1. > +pgoutput_pubgencol_init(PGOutputData *data, List *publications, > + RelationSyncEntry *entry) > > Can we name it as check_and_init_gencol? I don't know if it is a good > idea to append a prefix pgoutput for local functions. It is primarily > used for exposed functions from pgoutput.c. I see that in a few cases > we do that for local functions as well but that is not a norm. > > A related point: > + /* Initialize publish generated columns value */ > + pgoutput_pubgencol_init(data, rel_publications, entry); > > Accordingly change this comment to something like: "Check whether to > publish to generated columns.". > Fixed. > 2. > +/* > + * Returns true if the relation has column list associated with the > + * publication, false if the relation has no column list associated with the > + * publication. > + */ > +bool > +is_column_list_publication(Publication *pub, Oid relid) > ... > ... > > How about naming the above function as has_column_list_defined()? > Also, you can write the above comment as: "Returns true if the > relation has column list associated with the publication, false > otherwise." > Fixed. > 3. > + /* > + * The column list takes precedence over pubgencols, so skip checking > + * column list publications. > + */ > + if (is_column_list_publication(pub, entry->publish_as_relid)) > > Let's change this comment to: "The column list takes precedence over > publish_generated_columns option. Those will be checked later, see > pgoutput_column_list_init." > Fixed. The v43 version patch attached at [1] has the changes for the same. [1] - https://www.postgresql.org/message-id/CAHv8RjJJJRzy83tG0nB90ivYcp7sFKTU%3D_BcQ-nUZ7VbHFwceA%40mail.gmail.com Thanks and Regards, Shubham Khanna.
Re: Alias of VALUES RTE in explain plan
I wrote: > However ... I don't like this implementation, not even a little > bit. I forgot to mention a third problem, which is that reassigning the alias during subquery pullup means it doesn't happen if subquery pullup doesn't happen. As an example, with your patch: regression=# explain verbose select * from (values (1), (2)) v(x); QUERY PLAN Values Scan on v (cost=0.00..0.03 rows=2 width=4) Output: v.x (2 rows) regression=# explain verbose select * from (values (1), (random())) v(x); QUERY PLAN - Values Scan on "*VALUES*" (cost=0.00..0.03 rows=2 width=8) Output: "*VALUES*".column1 (2 rows) That's because the volatile function prevents subquery flattening. regards, tom lane
Re: cpluspluscheck complains about use of register
"Tristan Partin" writes: > FWIW, I ran into this compiling an extension written in C++ for v15, so > I'll throw my support for backpatching this if that is still on the > table. Understandable if not, though. I'm inclined to think "too late". Even if we back-patched to v15 and earlier now, your extension would probably still want to be compilable against earlier minor releases, so the back-patch would not help you a lot. Christoph's workaround of "#define register" is probably the best answer for old PG versions, or you can compile with "-Wno-register". regards, tom lane
Re: cpluspluscheck complains about use of register
On Fri Oct 25, 2024 at 3:19 PM CDT, Tom Lane wrote: "Tristan Partin" writes: FWIW, I ran into this compiling an extension written in C++ for v15, so I'll throw my support for backpatching this if that is still on the table. Understandable if not, though. I'm inclined to think "too late". Even if we back-patched to v15 and earlier now, your extension would probably still want to be compilable against earlier minor releases, so the back-patch would not help you a lot. Christoph's workaround of "#define register" is probably the best answer for old PG versions, or you can compile with "-Wno-register". For my purposes, I only need to support the latest minor releases of PG versions, so a backpatch would be useful come December (?). I do understand the "too late" argument though. We did indeed fix the problem with "-Wno-register," though it's always nice to not have the manual fix. But hey, Postgres is a C project, so it's all good 😄. Thanks for getting back to me, Tom. -- Tristan Partin Neon (https://neon.tech)
Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row
Hi! On Mon, 21 Oct 2024 at 17:39, Fujii Masao wrote: > > > > On 2024/10/21 18:30, Kirill Reshke wrote: > > v4 no longer applies. It now conflicts with > > e7834a1a251d4a28245377f383ff20a657ba8262. > > Also, there were review comments. > > > > So, I decided to rebase. > > Thanks for the patch! Here are my review comments: > > I noticed that on_error=set_to_null does not trigger NOTICE messages for rows > and columns with errors. It's "unexpected" thing for columns to be silently > replaced with NULL due to on_error=set_to_null. So, similar to > on_error=ignore, > there should be NOTICE messages indicating which input records had columns > set to NULL because of data type incompatibility. Without these messages, > users might not realize that some columns were set to NULL. Nice catch. That is a feature introduced by f5a227895e178bf528b18f82bbe554435fb3e64f. > > How should on_error=set_to_null behave when reject_limit is set? It seems > intuitive to trigger an error if the number of rows with columns' data type > issues exceeds reject_limit, similar to on_error=ignore. This is open to > discussion. Ok, let's discuss. My first suggestion was: when the REJECT LIMIT is set to some non-zero number and the number of row NULL replacements exceeds the limit, is it OK to fail. Because there WAS errors, and we should not tolerate more than $limit errors . I do find this behavior to be consistent. But what if we don't set a REJECT LIMIT, it is sane to do all replacements, as if REJECT LIMIT is inf. But our REJECT LIMIT is zero (not set). So, we ignore zero REJECT LIMIT if set_to_null is set. But while I was trying to implement that, I realized that I don't understand v4 of this patch. My misunderstanding is about `t_on_error_null` tests. We are allowed to insert a NULL value for the first column of t_on_error_null using COPY ON_ERROR SET_TO_NULL. Why do we do that? My thought is we should try to execute InputFunctionCallSafe with NULL value (i mean, here [1]) for the column after we failed to insert the input value. And, if this second call is successful, we do replacement, otherwise we count the row as erroneous. > > psql's tab-completion should be updated to include SET_TO_NULL. Agreed. > > > An error_action value of > stop means fail the command, while > ignore means discard the input row and continue > with the next one. > + set_to_null means the input value will be set to > null and continue with the next one. > > How about merging these two descriptions to one and updating it to the > following? > > --- > An error_action value of stop means fail the command, ignore means discard > the input > row and continue with the next one, and set_to_null means replace columns > with invalid > input values with NULL and move to the next row. > --- > Hm, good catch. Applied almost as you suggested. I did tweak this "replace columns with invalid input values with " into "replace columns containing erroneous input values with". Is that OK? >The ignore option is applicable only for > COPY FROM > > This should be "... ignore and set_to_null options are ..."? Sure! > Regards, > > -- > Fujii Masao > Advanced Computing Technology Center > Research and Development Headquarters > NTT DATA CORPORATION > Here I sent v6 where review comments, except set_to_null vs reject limit, were addressed. No new tests added yet, as some details are unclear... [1] https://github.com/postgresql-cfbot/postgresql/compare/cf/4810~1...cf/4810#diff-98d8bfd706468f77f8b0d5d0797e3dba3ffaaa88438143ef4cf7fedecaa56827R964 -- Best regards, Kirill Reshke v6-0001-on_error-set_to_null.patch Description: Binary data
Re: Statistics Import and Export
On Tue, 2024-09-17 at 05:02 -0400, Corey Huinker wrote: > > I've taken most of Jeff's work, reincorporated it into roughly the > same patch structure as before, and am posting it now. I have committed the import side of this patch series; that is, the function calls that can load stats into an existing cluster without the need to ANALYZE. The pg_restore_*_stats() functions are designed such that pg_dump can emit the calls. Some design choices of the functions worth noting: (a) a variadic signature of name/value pairs rather than ordinary SQL arguments, which makes it easier for future versions to interpret what has been output from a previous version; and (b) many kinds of errors are demoted to WARNINGs, to allow some statistics to be set for an attribute even if other statistics are malformed (also a future-proofing design); and (c) we are considering whether to use an in-place heap update for the relation stats, so that a large restore doesn't bloat pg_class -- I'd like feedback on this idea The pg_set_*_stats() functions are designed for interactive use, such as tweaking statistics for planner testing, experimentation, or reproducing a plan outside of a production system. The aforementioned design choices don't make a lot of sense in this context, so that's why the pg_set_*_stats() functions are separate from the pg_restore_*_stats() functions. But there's a lot of overlap, so it may be worth discussing again whether we should only have one set of functions. Regards, Jeff Davis
Re: Collation & ctype method table, and extension hooks
On Thu, 2024-10-24 at 10:05 +0200, Andreas Karlsson wrote:
> Why is there no pg_locale_builtin.c?
Just that it would be a fairly small file, but I'm fine with doing
that.
> I think adding an assert to create_pg_locale() which enforces valid
> there is always a combination of ctype_is_c and casemap would be
> good,
> similar to the collate field.
Good idea.
> Why are casemap and ctype_methods not the same struct? They seem very
> closely related.
The code impact was in fairly different places, so it seemed like a
nice way to break it out. I could combine them, but it would be a
fairly large patch.
> This commit makes me tempted to handle the ctype_is_c logic for
> character classes also in callbacks and remove the if in functions
> like
> pg_wc_ispunct(). But this si something that would need to be
> benchmarked.
That's a good idea. The reason collate_is_c is important is because
there are quite a few caller-specific optimizations, but that doesn't
seem to be true of ctype_is_c.
> I wonder if the bitmask idea isn't terrible for the branch predictor
> and
> that me may want one function per character class, but this is yet
> again
> something we need to benchmark.
Agreed -- a lot of work has gone into optimizing the regex code, and we
don't want a perf regression there. But I'm also not sure exactly which
kinds of tests I should be running for that.
> Is there a reason we allocate the icu_provider in
> create_pg_locale_icu
> with MemoryContextAllocZero when we intialize everything anyway? And
> similar for other providers.
Allocating and zeroing is a good defense against new optional methods
and fields which can safely default to zero.
> = v6-0011-Introduce-hooks-for-creating-custom-pg_locale_t.patch
>
> Looks good but seems like a quite painful API to use.
How is it painful and can we make it better?
> > * Have a CREATE LOCALE PROVIDER command and make "provider" an Oid
> > rather than a char ('b'/'i'/'c'). The v6 patches brings us close to
> > this point, but I'm not sure if we want to go this far in v18.
>
> Probably necessary but I hate all the DDL commands the way to SQL
> standard is written forces us to add.
There is some precedent for a DDL-like thing without new grammar:
pg_replication_origin_create(). I don't have a strong opinion on
whether to do that or not.
>
Regards,
Jeff Davis
Re: New function normal_rand_array function to contrib/tablefunc.
> 10). In this error:
>
> +elog(ERROR, "unsupported type %d for rand_array function.",
> + datatype);
>
> "datatype" is of type Oid, which is unsigned, and so it should use
> "%u" not "%d". Also, as above, it should not end with a period, so it
> should be:
>
> +elog(ERROR, "unsupported type %u for rand_array function",
> + datatype);
I remember my IDE could detect such issue before, but failed this
time. I just checked more today and it is still failing. Looks the
checker is not stable and I can't find out the reason so far.
main.c
#include
int main(int argc, char *argv[])
{
unsigned int i = 0;
int x = 2;
printf("i = %d\n", i);
printf("i = %u\n", x);
return 0;
}
All the following commands succeed without any warnings.
clang -O0 -g main.c -o main -Wall -Wformat
gcc -g main.c -o main -Wall -Wformat
scan-build clang -g main.c -o main -Wall -Wformat
cppcheck main.c
clang: 18.1.6
gcc: 13.3.0
Only "cppcheck --enable=all main.c" catch the warnning.
Any hints on this will be appreicated.
--
Best Regards
Andy Fan
