Re: MAINTAIN privilege -- what do we need to un-revert it?
On Thu, 01 Aug 2024 11:31:53 -0700
Jeff Davis wrote:
> On Wed, 2024-07-31 at 18:20 +0900, Yugo NAGATA wrote:
> > I agree that it might not be important, but I think adding the flag
> > would be
> > also helpful for improving code-readability because it clarify the
> > function
> > is used in the two cases. I attached patch for this fix (patch 0003).
>
> Committed with one minor modification: I moved the boolean flag to be
> near the other booleans rather than at the end. Thank you.
>
> > Sure. I fixed the patch to remove 'param' from both functions. (patch
> > 0002)
>
> Committed, thank you.
Thank you for committing them.
Should not they be backported to REL_17_STABLE?
>
> > I also add the small refactoring around ExecCreateTableAs(). (patch
> > 0001)
> >
> > - Remove matview-related codes from intorel_startup.
> > Materialized views are no longer handled in this function.
> >
> > - RefreshMatViewByOid is moved to just after create_ctas_nodata
> > call to improve code readability.
> >
>
> I'm not sure the changes in intorel_startup() are correct. I tried
> adding an Assert(into->viewQuery == NULL), and it fails because there's
> another path I did not consider: "EXPLAIN ANALYZE CREATE MATERIALIZED
> VIEW ...", which does not go through ExecCreateTableAs() but does go
> through CreateIntoRelDestReceiver().
>
> See:
>
> https://postgr.es/m/20444c382e6cb5e21e93c94d679d0198b0dba4dd.ca...@j-davis.com
>
> Should we refactor a bit and try to make EXPLAIN use the same code
> paths?
I overlooked that CreateIntoRelDestReceiver() is used from EXPLAIN. I saw the
thread above and I agree that we should refactor it to make EXPLAIN consistent
CREATE MATERIALIZED VIEW, but I suppose this should be discussed the other
thread.
I attached a updated patch removed the intorel_startup() part from.
Regards,
Yugo Nagata
--
Yugo NAGATA
>From 3609e85c288726e16dc851996a3b99e6179f43db Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Fri, 2 Aug 2024 16:02:07 +0900
Subject: [PATCH v3] Small refactoring around ExecCreateTableAs
Since commit b4da732f, the refresh logic is used to populate
materialized views, so we can simplify the error message
in ExecCreateTableAs.
Also, RefreshMatViewByOid is moved to just after create_ctas_nodata
call to improve code readability.
---
src/backend/commands/createas.c | 34 -
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 36e192b79b..c71ff80188 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -228,9 +228,6 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
bool do_refresh = false;
DestReceiver *dest;
ObjectAddress address;
- List *rewritten;
- PlannedStmt *plan;
- QueryDesc *queryDesc;
/* Check if the relation exists or not */
if (CreateTableAsRelExists(stmt))
@@ -279,9 +276,25 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
* from running the planner before all dependencies are set up.
*/
address = create_ctas_nodata(query->targetList, into);
+
+ /*
+ * For materialized views, reuse the REFRESH logic, which locks down
+ * security-restricted operations and restricts the search_path. This
+ * reduces the chance that a subsequent refresh will fail.
+ */
+ if (do_refresh)
+ RefreshMatViewByOid(address.objectId, true, false, false,
+pstate->p_sourcetext, qc);
+
}
else
{
+ List *rewritten;
+ PlannedStmt *plan;
+ QueryDesc *queryDesc;
+
+ Assert(!is_matview);
+
/*
* Parse analysis was done already, but we still have to run the rule
* rewriter. We do not do AcquireRewriteLocks: we assume the query
@@ -292,9 +305,7 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
/* SELECT should never rewrite to more or less than one SELECT query */
if (list_length(rewritten) != 1)
- elog(ERROR, "unexpected rewrite result for %s",
- is_matview ? "CREATE MATERIALIZED VIEW" :
- "CREATE TABLE AS SELECT");
+ elog(ERROR, "unexpected rewrite result for CREATE TABLE AS SELECT");
query = linitial_node(Query, rewritten);
Assert(query->commandType == CMD_SELECT);
@@ -339,17 +350,6 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
PopActiveSnapshot();
}
- /*
- * For materialized views, reuse the REFRESH logic, which locks down
- * security-restricted operations and restricts the search_path. This
- * reduces the chance that a subsequent refresh will fail.
- */
- if (do_refresh)
- {
- RefreshMatViewByOid(address.objectId, true, false, false,
- pstate->p_sourcetext, qc);
- }
-
return address;
}
--
2.34.1
Re: Adding OLD/NEW support to RETURNING
On Thu, Aug 1, 2024 at 7:33 PM Dean Rasheed wrote:
>
> On Mon, 29 Jul 2024 at 11:22, Dean Rasheed wrote:
> >
> > Trivial rebase, following c7301c3b6f.
> >
>
> Rebased version, forced by a7f107df2b. Evaluating the input parameters
> of correlated SubPlans in the referencing ExprState simplifies this
> patch in a couple of places, since it no longer has to worry about
> copying ExprState flags to a new ExprState.
>
hi. some minor issues.
saveOld = changingPart && resultRelInfo->ri_projectReturning &&
resultRelInfo->ri_projectReturning->pi_state.flags & EEO_FLAG_HAS_OLD;
if (resultRelInfo->ri_projectReturning && (processReturning || saveOld))
{
}
"saveOld" imply "resultRelInfo->ri_projectReturning"
we can simplified it as
if (processReturning || saveOld))
{
}
for projectReturning->pi_state.flags,
we don't use EEO_FLAG_OLD_IS_NULL, EEO_FLAG_NEW_IS_NULL
in ExecProcessReturning, we can do the following way.
/* Make old/new tuples available to ExecProject, if required */
if (oldSlot)
econtext->ecxt_oldtuple = oldSlot;
else if (projectReturning->pi_state.flags & EEO_FLAG_HAS_OLD)
econtext->ecxt_oldtuple = ExecGetAllNullSlot(estate, resultRelInfo);
else
econtext->ecxt_oldtuple = NULL; /* No references to OLD columns */
if (newSlot)
econtext->ecxt_newtuple = newSlot;
else if (projectReturning->pi_state.flags & EEO_FLAG_HAS_NEW)
econtext->ecxt_newtuple = ExecGetAllNullSlot(estate, resultRelInfo);
else
econtext->ecxt_newtuple = NULL; /* No references to NEW columns */
/*
* Tell ExecProject whether or not the OLD/NEW rows exist (needed for any
* ReturningExpr nodes).
*/
if (oldSlot == NULL)
projectReturning->pi_state.flags |= EEO_FLAG_OLD_IS_NULL;
else
projectReturning->pi_state.flags &= ~EEO_FLAG_OLD_IS_NULL;
if (newSlot == NULL)
projectReturning->pi_state.flags |= EEO_FLAG_NEW_IS_NULL;
else
projectReturning->pi_state.flags &= ~EEO_FLAG_NEW_IS_NULL;
@@ -2620,6 +2620,13 @@ transformWholeRowRef(ParseState *pstate,
* point, there seems no harm in expanding it now rather than during
* planning.
*
+ * Note that if the nsitem is an OLD/NEW alias for the target RTE (as can
+ * appear in a RETURNING list), its alias won't match the target RTE's
+ * alias, but we still want to make a whole-row Var here rather than a
+ * RowExpr, for consistency with direct references to the target RTE, and
+ * so that any dropped columns are handled correctly. Thus we also check
+ * p_returning_type here.
+ *
makeWholeRowVar and subroutines only related to pg_type, but dropped
column info is in pg_attribute.
I don't understand "so that any dropped columns are handled correctly".
ExecEvalSysVar, slot_getsysattr we have "Assert(attnum < 0);"
but
ExecEvalSysVar, while rowIsNull is true, we didn't do "Assert(attnum < 0);"
Re: why there is not VACUUM FULL CONCURRENTLY?
On Fri, 2 Aug 2024 at 11:09, Antonin Houska wrote: > > Kirill Reshke wrote: > > However, in general, the 3rd patch is really big, very hard to > > comprehend. Please consider splitting this into smaller (and > > reviewable) pieces. > > I'll try to move some preparation steps into separate diffs, but not sure if > that will make the main diff much smaller. I prefer self-contained patches, as > also explained in [3]. Thanks for sharing [3], it is a useful link. There is actually one more case when ACCESS EXCLUSIVE is held: during table rewrite (AT set TAM, AT set Tablespace and AT alter column type are some examples). This can be done CONCURRENTLY too, using the same logical replication approach, or do I miss something? I'm not saying we must do it immediately, this should be a separate thread, but we can do some preparation work here. I can see that a bunch of functions which are currently placed in cluster.c can be moved to something like logical_rewrite_heap.c. ConcurrentChange struct and apply_concurrent_insert function is one example of such. So, if this is the case, 0003 patch can be splitted in two: The first one is general utility code for logical table rewrite The second one with actual VACUUM CONCURRENTLY feature. What do you think?
Re: [Patch] remove duplicated smgrclose
Thanks, I have set my name in the Authors column of CF. Steven Junwang Zhao 于2024年8月2日周五 13:22写道： > Hi Steven, > > On Fri, Aug 2, 2024 at 12:12 PM Steven Niu wrote: > > > > Hi, Junwang, > > > > Thank you for the review and excellent summary in commit message! > > > > This is my first contribution to community, and not so familiar with the > overall process. > > After reading the process again, it looks like that I'm not qualified to > submit the patch to commitfest as I never had reviewed others' work. :( > > If so, could you please help to submit it to commitfest? > > > > https://commitfest.postgresql.org/49/5149/ > > I can not find your profile on commitfest so I left the author as empty, > have you ever registered? If you have a account, you can put your > name in the Authors list. > > > Best Regards, > > Steven > > > > Junwang Zhao 于2024年8月1日周四 20:32写道： > >> > >> Hi Steven, > >> > >> On Wed, Jul 31, 2024 at 11:16 AM Steven Niu wrote: > >> > > >> > Hello, hackers, > >> > > >> > I think there may be some duplicated codes. > >> > Function smgrDoPendingDeletes() calls both smgrdounlinkall() and > smgrclose(). > >> > But both functions would close SMgrRelation object, it's dupliacted > behavior? > >> > > >> > So I make this patch. Could someone take a look at it? > >> > > >> > Thanks for your help, > >> > Steven > >> > > >> > From Highgo.com > >> > > >> > > >> You change LGTM, but the patch seems not to be applied to HEAD, > >> I generate the attached v2 using `git format` with some commit message. > >> > >> -- > >> Regards > >> Junwang Zhao > > > > -- > Regards > Junwang Zhao >
Set query_id for query contained in utility statement
Hi all, Some utility statements like Explain, CreateTableAs and DeclareCursor contain a query which will be planned and executed. During post parse, only the top utility statement is jumbled, leaving the contained query without a query_id set. Explain does the query jumble in ExplainQuery but for the contained query but CreateTableAs and DeclareCursor were left with unset query_id. This leads to extensions relying on query_id like pg_stat_statements to not be able to track those nested queries as the query_id was 0. This patch fixes this by recursively jumbling queries contained in those utility statements during post_parse, setting the query_id for those contained queries and removing the need for ExplainQuery to do it for the Explain statements. Regards, Anthonin Bonnefoy v1-0001-Set-query_id-for-queries-contained-in-utility-sta.patch Description: Binary data
Small refactoring around vacuum_open_relation
I hate to be "that guy", but there are many places in sources where we use LOCKMODE lockmode; variable and exactly one where we use LOCKMODE lmode: it is vacuum_open_relation function. Is it worth a patch? v1-0001-Rename-vacuum_open_relation-argument-to-lockmode.patch Description: Binary data
Re: Logical Replication of sequences
On Thu, Aug 1, 2024 at 9:26 AM shveta malik wrote: > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote: > > > > Thanks for reporting this, these issues are fixed in the attached > > v20240730_2 version patch. > > I was reviewing the design of patch003, and I have a query. Do we need to even start an apply worker and create replication slot when subscription created is for 'sequences only'? IIUC, currently logical replication apply worker is the one launching sequence-sync worker whenever needed. I think it should be the launcher doing this job and thus apply worker may even not be needed for current functionality of sequence sync? Going forward when we implement incremental sync of sequences, then we may have apply worker started but now it is not needed. thanks Shveta
Re: v17 vs v16 performance comparison
01.08.2024 06:41, Tom Lane wrote:
But beside that, I've found a separate regression. Bisecting for this
degradation:
Best pg-src-17--.* worse than pg-src-16--.* by 105.0 percents (356.63 >
173.96): s64da_tpcds.query95
Average pg-src-17--.* worse than pg-src-16--.* by 105.2 percents (357.79 >
174.38): s64da_tpcds.query95
pointed at f7816aec2.
I'm not terribly concerned about that. The nature of planner changes
like that is that some queries will get worse and some better, because
the statistics and cost estimates we're dealing with are not perfect.
It is probably worth drilling down into that test case to understand
where the planner is going wrong, with an eye to future improvements;
but I doubt it's something we need to address for v17.
Please find attached two plans for that query [1].
(I repeated the benchmark for f7816aec2 and f7816aec2~1 five times and
made sure that both plans are stable.)
Meanwhile I've bisected another degradation:
Best pg-src-17--.* worse than pg-src-16--.* by 11.3 percents (7.17 > 6.44):
job.query6f
and came to the commit b7b0f3f27 again.
[1]
https://github.com/swarm64/s64da-benchmark-toolkit/blob/master/benchmarks/tpcds/queries/queries_10/95.sql
Best regards,
Alexander[{
"Query Text": "-- RNGSEED: 1\n\n-- EXPLAIN (FORMAT JSON)\nwith ws_wh
as\n(select ws1.ws_order_number,ws1.ws_warehouse_sk wh1,ws2.ws_warehouse_sk
wh2\n from web_sales ws1,web_sales ws2\n where ws1.ws_order_number =
ws2.ws_order_number\n and ws1.ws_warehouse_sk <> ws2.ws_warehouse_sk)\n
select\n count(distinct ws_order_number) as \"order count\"\n
,sum(ws_ext_ship_cost) as \"total shipping cost\"\n ,sum(ws_net_profit) as
\"total net profit\"\nfrom\n web_sales ws1\n ,date_dim\n
,customer_address\n ,web_site\nwhere\nd_date between '2000-3-01' and\n
(cast('2000-3-01' as date) + 60)\nand ws1.ws_ship_date_sk =
d_date_sk\nand ws1.ws_ship_addr_sk = ca_address_sk\nand ca_state = 'GA'\nand
ws1.ws_web_site_sk = web_site_sk\nand web_company_name = 'pri'\nand
ws1.ws_order_number in (select ws_order_number\n
from ws_wh)\nand ws1.ws_order_number in (select wr_order_number\n
from web_returns,ws_wh\nwhere
wr_order_number = ws_wh.ws_order_number)\norder by count(distinct
ws_order_number)\nlimit 100;\n",
"Plan": {
"Node Type": "Limit",
"Parallel Aware": false,
"Async Capable": false,
"Startup Cost": 1244768.73,
"Total Cost": 1244768.73,
"Plan Rows": 1,
"Plan Width": 72,
"Actual Startup Time": 277228.286,
"Actual Total Time": 277228.294,
"Actual Rows": 1,
"Actual Loops": 1,
"Output": ["(count(DISTINCT ws1.ws_order_number))",
"(sum(ws1.ws_ext_ship_cost))", "(sum(ws1.ws_net_profit))"],
"Plans": [
{
"Node Type": "Hash Join",
"Parent Relationship": "InitPlan",
"Subplan Name": "CTE ws_wh",
"Parallel Aware": false,
"Async Capable": false,
"Join Type": "Inner",
"Startup Cost": 76039.92,
"Total Cost": 443675.28,
"Plan Rows": 17135538,
"Plan Width": 12,
"Actual Startup Time": 380.616,
"Actual Total Time": 3160.748,
"Actual Rows": 13341520,
"Actual Loops": 1,
"Output": ["ws1_1.ws_order_number", "ws1_1.ws_warehouse_sk",
"ws2.ws_warehouse_sk"],
"Inner Unique": false,
"Hash Cond": "(ws1_1.ws_order_number = ws2.ws_order_number)",
"Join Filter": "(ws1_1.ws_warehouse_sk <> ws2.ws_warehouse_sk)",
"Rows Removed by Join Filter": 4787014,
"Plans": [
{
"Node Type": "Seq Scan",
"Parent Relationship": "Outer",
"Parallel Aware": false,
"Async Capable": false,
"Relation Name": "web_sales",
"Schema": "public",
"Alias": "ws1_1",
"Startup Cost": 0.00,
"Total Cost": 52382.52,
"Plan Rows": 1441952,
"Plan Width": 8,
"Actual Startup Time": 0.013,
"Actual Total Time": 223.129,
"Actual Rows": 1441948,
"Actual Loops": 1,
"Output": ["ws1_1.ws_order_number", "ws1_1.ws_warehouse_sk"]
},
{
"Node Type": "Hash",
"Parent Relationship": "Inner",
"Parallel Aware": false,
"Async Capable": false,
"Startup Cost": 52382.52,
"Total Cost": 52382.52,
"Plan Rows": 1441952,
"Plan Width": 8,
"Actual Startup Time": 379.625,
"Actual Total Time": 379.626,
"Actual Rows": 1441948,
"Actual Loops": 1,
"Output": ["ws2.ws_warehouse_sk", "ws2.ws_order_number"],
"Hash Buckets": 262144,
"Original Hash Buckets": 262144,
"Hash Batches": 16,
"Original Hash Batches": 16,
"Peak Memory Usage": 5602,
"Plans"
Re: Logical Replication of sequences
On Fri, Aug 2, 2024 at 2:24 PM shveta malik wrote: > > On Thu, Aug 1, 2024 at 9:26 AM shveta malik wrote: > > > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote: > > > > > > Thanks for reporting this, these issues are fixed in the attached > > > v20240730_2 version patch. > > > > > I was reviewing the design of patch003, and I have a query. Do we need > to even start an apply worker and create replication slot when > subscription created is for 'sequences only'? IIUC, currently logical > replication apply worker is the one launching sequence-sync worker > whenever needed. I think it should be the launcher doing this job and > thus apply worker may even not be needed for current functionality of > sequence sync? Going forward when we implement incremental sync of > sequences, then we may have apply worker started but now it is not > needed. > Also, can we please mention the state change and 'who does what' atop sequencesync.c file similar to what we have atop tablesync.c file otherwise it is difficult to figure out the flow. thanks Shveta
Re: [PATCH] Add min/max aggregate functions to BYTEA
> On 24 Jul 2024, at 17:42, Marat Bukharov wrote: > > V5 patch. I've added more tests with different bytea sizes Hi Marat! This looks like a nice feature to have. I’ve took a look into the patch and have few suggestions: 0. Please write more descriptive commit message akin to [0] 1. Use oids from development range 8000- 2. Replace VARDATA_ANY\memcmp dance with a call to varstrfastcmp_c(). Thank you! Best regards, Andrey Borodin. [0] https://github.com/postgres/postgres/commit/a0f1fce80c03
Re: Small refactoring around vacuum_open_relation
On Fri, Aug 2, 2024 at 1:55 PM Kirill Reshke wrote: > > I hate to be "that guy", but there are many places in sources where we use > LOCKMODE lockmode; variable and exactly one where we use LOCKMODE > lmode: it is vacuum_open_relation function. There are more instances of LOCKMODE lmode; I spotted one in plancat.c as well. Case1: toast_get_valid_index(Oid toastoid, LOCKMODE lock) toast_close_indexes(Relation *toastidxs, int num_indexes, LOCKMODE lock) GetLockmodeName(LOCKMETHODID lockmethodid, LOCKMODE mode) LOCKMODE mode = 0; Case 2: qualified variable names like LOCKMODE heapLockmode; LOCKMODE memlockmode; LOCKMODE table_lockmode; LOCKMODE parentLockmode; LOCKMODE cmd_lockmode = AccessExclusiveLock; /* default for compiler */ LOCK_PRINT(const char *where, const LOCK *lock, LOCKMODE type) case3: some that have two LOCKMODE instances like DoLockModesConflict(LOCKMODE mode1, LOCKMODE mode2) > Is it worth a patch? When I see a variable with name lockmode, I know it's of type LOCKMODE. So changing the Case1 may be worth it. It's not a whole lot of code churn as well. May be patch backbranches. Case2 we should leave as is since the variable name has lockmode in it. Case3, worth changing to lockmode1 and lockmode2. -- Best Wishes, Ashutosh Bapat
Re: Wrong results with grouping sets
I've been looking at cases where there are grouping-set keys that reduce to Consts, and I noticed a plan with v11 patch that is not very great. explain (verbose, costs off) select 1 as one group by rollup(one) order by one nulls first; QUERY PLAN --- Sort Output: (1) Sort Key: (1) NULLS FIRST -> GroupAggregate Output: (1) Group Key: (1) Group Key: () -> Sort Output: (1) Sort Key: (1) -> Result Output: 1 (12 rows) The Sort operation below the Agg node is unnecessary because the grouping key is actually a Const. This plan results from wrapping the Const in a PlaceHolderVar to carry the nullingrel bit of the RTE_GROUP RT index, as it can be nulled by the grouping step. Although we remove this nullingrel bit when generating the groupClause pathkeys since we know the groupClause is logically below the grouping step, we do not unwrap the PlaceHolderVar. This suggests that we might need a mechanism to unwrap PHVs when safe. 0003 includes a flag in PlaceHolderVar to indicate whether it is safe to remove the PHV and use its contained expression instead when its phnullingrels becomes empty. Currently it is set true only in cases where the PHV is used to carry the nullingrel bit of the RTE_GROUP RT index. With 0003 the plan above becomes more reasonable: explain (verbose, costs off) select 1 as one group by rollup(one) order by one nulls first; QUERY PLAN - Sort Output: (1) Sort Key: (1) NULLS FIRST -> GroupAggregate Output: (1) Group Key: 1 Group Key: () -> Result Output: 1 (9 rows) This could potentially open up opportunities for optimization by unwrapping PHVs in other cases. As an example, consider explain (costs off) select * from t t1 left join lateral (select t1.a as x, * from t t2) s on true where t1.a = s.a; QUERY PLAN Nested Loop -> Seq Scan on t t1 -> Seq Scan on t t2 Filter: (t1.a = a) (4 rows) The target entry s.x is wrapped in a PHV that contains lateral reference to t1, which forces us to resort to nestloop join. However, since the left join has been reduced to an inner join, we should be able to remove this PHV and use merge or hash joins instead. I did not implement this optimization in 0003. It seems that it should be addressed in a separate patch. Thanks Richard v12-0001-Introduce-an-RTE-for-the-grouping-step.patch Description: Binary data v12-0002-Mark-expressions-nullable-by-grouping-sets.patch Description: Binary data v12-0003-Unwrap-a-PlaceHolderVar-when-safe.patch Description: Binary data
Re: Small refactoring around vacuum_open_relation
Thanks for review! On Fri, 2 Aug 2024 at 14:31, Ashutosh Bapat wrote: > > On Fri, Aug 2, 2024 at 1:55 PM Kirill Reshke wrote: > > > > I hate to be "that guy", but there are many places in sources where we use > > LOCKMODE lockmode; variable and exactly one where we use LOCKMODE > > lmode: it is vacuum_open_relation function. > > There are more instances of LOCKMODE lmode; I spotted one in plancat.c as > well. Nice catch! > Case1: > toast_get_valid_index(Oid toastoid, LOCKMODE lock) > toast_close_indexes(Relation *toastidxs, int num_indexes, LOCKMODE lock) > GetLockmodeName(LOCKMETHODID lockmethodid, LOCKMODE mode) > LOCKMODE mode = 0; > Case 2: > qualified variable names like > LOCKMODE heapLockmode; > LOCKMODE memlockmode; > LOCKMODE table_lockmode; > LOCKMODE parentLockmode; > LOCKMODE cmd_lockmode = AccessExclusiveLock; /* default for compiler */ > LOCK_PRINT(const char *where, const LOCK *lock, LOCKMODE type) > > case3: some that have two LOCKMODE instances like > DoLockModesConflict(LOCKMODE mode1, LOCKMODE mode2) Nice catch! > > Is it worth a patch? > > When I see a variable with name lockmode, I know it's of type > LOCKMODE. So changing the Case1 may be worth it. It's not a whole lot > of code churn as well. May be patch backbranches. > > Case2 we should leave as is since the variable name has lockmode in it. +1 > Case3, worth changing to lockmode1 and lockmode2. Agree > -- > Best Wishes, > Ashutosh Bapat Attached v2 patch with your suggestions addressed. v2-0001-Rename-LOCKMODE-type-vairables-to-lockmode.patch Description: Binary data
Remove obsolete RECHECK keyword completely
I propose to remove the obsolete RECHECK keyword completely. This used
to be part of CREATE OPERATOR CLASS and ALTER OPERATOR FAMILY, but it
has done nothing (except issue a NOTICE) since PostgreSQL 8.4. Commit
30e7c175b81 removed support for dumping from pre-9.2 servers, so this no
longer serves any need, it seems to me.
This now removes it completely, and you'd get a normal parse error if
you used it.
From 284e0a8f4729d9d6507ae0029cad637b7d870d41 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Fri, 2 Aug 2024 12:37:33 +0200
Subject: [PATCH] Remove obsolete RECHECK keyword completely
This used to be part of CREATE OPERATOR CLASS and ALTER OPERATOR
FAMILY, but it has done nothing (except issue a NOTICE) since
PostgreSQL 8.4. Commit 30e7c175b81 removed support for dumping from
pre-9.2 servers, so this no longer serves any need.
This now removes it completely, and you'd get a normal parse error if
you used it.
---
doc/src/sgml/ref/alter_opfamily.sgml | 8 --
doc/src/sgml/ref/create_opclass.sgml | 8 --
src/backend/parser/gram.y | 26 +++
src/include/parser/kwlist.h | 1 -
.../isolation/specs/merge-match-recheck.spec | 2 +-
5 files changed, 4 insertions(+), 41 deletions(-)
diff --git a/doc/src/sgml/ref/alter_opfamily.sgml
b/doc/src/sgml/ref/alter_opfamily.sgml
index b2e5b9b72ec..5c4c2e579f5 100644
--- a/doc/src/sgml/ref/alter_opfamily.sgml
+++ b/doc/src/sgml/ref/alter_opfamily.sgml
@@ -273,14 +273,6 @@ Notes
is likely to be inlined into the calling query, which will prevent
the optimizer from recognizing that the query matches an index.
-
-
- Before PostgreSQL 8.4, the
OPERATOR
- clause could include a RECHECK option. This is no longer
- supported because whether an index operator is lossy is now
- determined on-the-fly at run time. This allows efficient handling of
- cases where an operator might or might not be lossy.
-
diff --git a/doc/src/sgml/ref/create_opclass.sgml
b/doc/src/sgml/ref/create_opclass.sgml
index f1d6a4cbbe2..2d560b68658 100644
--- a/doc/src/sgml/ref/create_opclass.sgml
+++ b/doc/src/sgml/ref/create_opclass.sgml
@@ -269,14 +269,6 @@ Notes
is likely to be inlined into the calling query, which will prevent
the optimizer from recognizing that the query matches an index.
-
-
- Before PostgreSQL 8.4, the
OPERATOR
- clause could include a RECHECK option. This is no longer
- supported because whether an index operator is lossy is now
- determined on-the-fly at run time. This allows efficient handling of
- cases where an operator might or might not be lossy.
-
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a043fd4c669..c3f25582c38 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -495,7 +495,7 @@ static Node *makeRecursiveViewSelect(char *relname, List
*aliases, Node *query);
%type opt_instead
%type opt_unique opt_verbose opt_full
-%type opt_freeze opt_analyze opt_default opt_recheck
+%type opt_freeze opt_analyze opt_default
%type opt_binary copy_delimiter
%type copy_from opt_program
@@ -770,7 +770,7 @@ static Node *makeRecursiveViewSelect(char *relname, List
*aliases, Node *query);
QUOTE QUOTES
- RANGE READ REAL REASSIGN RECHECK RECURSIVE REF_P REFERENCES REFERENCING
+ RANGE READ REAL REASSIGN RECURSIVE REF_P REFERENCES REFERENCING
REFRESH REINDEX RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA
RESET RESTART RESTRICT RETURN RETURNING RETURNS REVOKE RIGHT ROLE
ROLLBACK ROLLUP
ROUTINE ROUTINES ROW ROWS RULE
@@ -6622,7 +6622,7 @@ opclass_item_list:
;
opclass_item:
- OPERATOR Iconst any_operator opclass_purpose opt_recheck
+ OPERATOR Iconst any_operator opclass_purpose
{
CreateOpClassItem *n =
makeNode(CreateOpClassItem);
ObjectWithArgs *owa =
makeNode(ObjectWithArgs);
@@ -6636,7 +6636,6 @@ opclass_item:
$$ = (Node *) n;
}
| OPERATOR Iconst operator_with_argtypes opclass_purpose
- opt_recheck
{
CreateOpClassItem *n =
makeNode(CreateOpClassItem);
@@ -6688,23 +6687,6 @@ opclass_purpose: FOR SEARCH
{ $$ = NIL; }
| /*EMPTY*/
{ $$ = NIL; }
;
-opt_recheck: RECHECK
- {
- /*
-* RECHECK no longer does anything in
opclass definitions,
-* but we still accept it to ease
porting o
Re: Conflict detection and logging in logical replication
On Thu, Aug 1, 2024 at 5:23 PM Amit Kapila wrote: > > On Thu, Aug 1, 2024 at 2:26 PM Hayato Kuroda (Fujitsu) > wrote: > > > > 04. general > > > > According to the documentation [1], there is another constraint "exclude", > > which > > can cause another type of conflict. But this pattern cannot be logged in > > detail. > > > > As per docs, "exclusion constraints can specify constraints that are > more general than simple equality", so I don't think it satisfies the > kind of conflicts we are trying to LOG and then in the future patch > allows automatic resolution for the same. For example, when we have > last_update_wins strategy, we will replace the rows with remote rows > when the key column values match which shouldn't be true in general > for exclusion constraints. Similarly, we don't want to consider other > constraint violations like CHECK to consider as conflicts. We can > always extend the basic functionality for more conflicts if required > but let's go with reporting straight-forward stuff first. > It is better to document that exclusion constraints won't be supported. We can even write a comment in the code and or commit message that we can extend it in the future. * + * Return true if the commit timestamp data was found, false otherwise. + */ +bool +GetTupleCommitTs(TupleTableSlot *localslot, TransactionId *xmin, + RepOriginId *localorigin, TimestampTz *localts) This API returns both xmin and commit timestamp, so wouldn't it be better to name it as GetTupleTransactionInfo or something like that? I have made several changes in the attached top-up patch. These include changes in the comments, docs, function names, etc. -- With Regards, Amit Kapila. diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml index c1f6f1aaa8..dfbff3de02 100644 --- a/doc/src/sgml/logical-replication.sgml +++ b/doc/src/sgml/logical-replication.sgml @@ -1585,17 +1585,17 @@ test_sub=# SELECT * FROM t1 ORDER BY id; Additional logging is triggered in the following conflict - scenarios: + cases: insert_exists Inserting a row that violates a NOT DEFERRABLE - unique constraint. Note that to obtain the origin and commit - timestamp details of the conflicting key in the log, ensure that + unique constraint. Note that to log the origin and commit + timestamp details of the conflicting key, track_commit_timestamp - is enabled. In this scenario, an error will be raised until the + should be enabled. In this case, an error will be raised until the conflict is resolved manually. @@ -1605,10 +1605,10 @@ test_sub=# SELECT * FROM t1 ORDER BY id; The updated value of a row violates a NOT DEFERRABLE - unique constraint. Note that to obtain the origin and commit - timestamp details of the conflicting key in the log, ensure that + unique constraint. Note that to log the origin and commit + timestamp details of the conflicting key, track_commit_timestamp - is enabled. In this scenario, an error will be raised until the + is enabled. In this case, an error will be raised until the conflict is resolved manually. Note that when updating a partitioned table, if the updated row value satisfies another partition constraint resulting in the row being inserted into a @@ -1720,10 +1720,10 @@ CONTEXT: processing remote data for replication origin "pg_16395" during "INSER commit timestamp can be seen in the DETAIL line of the log. But note that this information is only available when track_commit_timestamp - is enabled. Users can use these information to make decisions on whether to - retain the local change or adopt the remote alteration. For instance, the - DETAIL line in above log indicates that the existing row was - modified by a local change, users can manually perform a remote-change-win + is enabled. Users can use this information to decide whether to retain the + local change or adopt the remote alteration. For instance, the + DETAIL line in the above log indicates that the existing + row was modified locally. Users can manually perform a remote-change-win resolution by deleting the local row. diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index ad3eda1459..34050a3bae 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -475,13 +475,13 @@ retry: } /* - * Find the tuple that violates the passed in unique index constraint - * (conflictindex). + * Find the tuple that violates the passed unique index (conflictindex). * - * If no conflict is found, return false and set *conflictslot to NULL. - * Otherwise return true, and the conflicting tuple is locked and returned in - * *conflictslot. The lock is essential to allow retrying to find the - * conflicting tu
Re: Memory growth observed with C++ application consuming libpq.dll on Windows
Hi Yasir, Are you looking for a fully functional sample program or only the APIs from libpq library that our product uses? I am asking this because if the requirement is to have a sample code, then I will have to work on creating one on the same lines as our product. Rajesh On Thu, Aug 1, 2024 at 5:27 PM Yasir wrote: > Hi Rajesh, > > Can you please attach a sample code snippet showing libpq's functions > being called? It will help to identify the libpq's functions to investigate > further for a potential mem leak. > > Regards... > > Yasir Hussain > > On Thu, Aug 1, 2024 at 4:30 PM Rajesh Kokkonda > wrote: > >> Hi, >> >> We are seeing a gradual growth in the memory consumption of our process >> on Windows. Ours is a C++ application that directly loads libpq.dll and >> handles the queries and functions. We use setSingleRowMethod to limit the >> number of rows returned simultaneously to the application. We do not >> observe any memory increase when the application is run on Linux. There is >> no code difference between Windows and Linux from the >> application standpoint. We ran valgrind against our application on Linux >> and found no memory leaks. Since the same code is being used on Windows as >> well, we do not suspect any memory leak there. The question is if there >> are any known memory leaks with the version of the library we are using on >> Windows. Kindly let us know. >> >> The version of the library on Linux is libpq.so.5.16 >> >> The windows version of the library is 16.0.3.0 >> >> >> [image: image.png] >> >> Thanks, >> Rajesh >> >
Re: Memory growth observed with C++ application consuming libpq.dll on Windows
On Fri, Aug 2, 2024 at 1:53 PM Rajesh Kokkonda wrote: > Hi Yasir, > > Are you looking for a fully functional sample program or only the APIs > from libpq library that our product uses? I am asking this because if the > requirement is to have a sample code, then I will have to work on creating > one on the same lines as our product. > > A functional sample is always best and preferred, however, only APIs used by your product would also be sufficient. Rajesh > > On Thu, Aug 1, 2024 at 5:27 PM Yasir wrote: > >> Hi Rajesh, >> >> Can you please attach a sample code snippet showing libpq's functions >> being called? It will help to identify the libpq's functions to investigate >> further for a potential mem leak. >> >> Regards... >> >> Yasir Hussain >> >> On Thu, Aug 1, 2024 at 4:30 PM Rajesh Kokkonda < >> rajeshk.kokko...@gmail.com> wrote: >> >>> Hi, >>> >>> We are seeing a gradual growth in the memory consumption of our process >>> on Windows. Ours is a C++ application that directly loads libpq.dll and >>> handles the queries and functions. We use setSingleRowMethod to limit the >>> number of rows returned simultaneously to the application. We do not >>> observe any memory increase when the application is run on Linux. There is >>> no code difference between Windows and Linux from the >>> application standpoint. We ran valgrind against our application on Linux >>> and found no memory leaks. Since the same code is being used on Windows as >>> well, we do not suspect any memory leak there. The question is if there >>> are any known memory leaks with the version of the library we are using on >>> Windows. Kindly let us know. >>> >>> The version of the library on Linux is libpq.so.5.16 >>> >>> The windows version of the library is 16.0.3.0 >>> >>> >>> [image: image.png] >>> >>> Thanks, >>> Rajesh >>> >>
Re: Psql meta-command conninfo+
Hi,
I have read the entire thread discussion. I understood the importance of
this enhancement related to /conninfo+ meta command. I really appreciate
the efforts of Maiquel and suggestions made by the reviewers. According to
best of my understanding, libpq API should be used instead of creating
server query for conninfo+ meta command. Building on the patch (v29)
provided by Maiquel, I made changes to retrieve some extra information
related to connection from libpq API.
Extra information includes:
- Protocol Version
- SSL Connection
- GSSAPI Authenticated
- Client Encoding
- Server Encoding
- Session User
- Backend PID
Output of \conninfo+:
1.
$ bin/psql --port=5430 postgres -h localhost
psql (18devel)
Type "help" for help.
postgres=# \conninfo+
You are connected to database "postgres" as user "hunaid" on host
"localhost" (address "127.0.0.1") at port "5430".
Protocol Version: 3
SSL Connection: no
GSSAPI Authenticated: no
Client Encoding: UTF8
Server Encoding: UTF8
Session User: hunaid
Backend PID: 163816
I have also edited the documentation and added it to the patch. Please let
me know if any changes are required.
Regards,
Hunaid Sohail
On Wed, Jun 5, 2024 at 5:32â¯PM Maiquel Grassi wrote:
> From a quick skim of the latest messages in this thread, it sounds like
> there are a couple of folks who think \conninfo (and consequently
> \conninfo+) should only report values from the libpq API. I think they
> would prefer server-side information to be provided via a system view or
> maybe an entirely new psql meta-command.
>
> IIUC a new system view would more-or-less just gather information from
> other system views and functions. A view would be nice because it could be
> used outside psql, but I'm not sure to what extent we are comfortable
> adding system views built on other system views. Something like
> \sessioninfo (as proposed by Sami) would look more similar to what you have
> in your latest patch, i.e., we'd teach psql about a complicated query for
> gathering all this disparate information.
>
> IMHO a good way to help move this forward is to try implementing it as a
> system view so that we can compare the two approaches. I've had luck in
> the past with implementing something a couple different ways to help drive
> consensus.
>
> --//--
>
> Great Nathan, we can follow that path of the view. I leave it open for
> anyone in the thread who has been following from the beginning to start the
> development of the view. Even you, if you have the interest and time to do
> it. At this exact moment, I am involved in a "my baby born" project, so I
> am unable to stop to look into this. If someone wants to start, I would
> appreciate it.
> Regards,
> Maiquel Grassi.
>
ÿþd i f f - - g i t a / d o c / s r c / s g m l / r e f / p s q l - r e f . s g m l b / d o c / s r c / s g m l / r e f / p s q l - r e f . s g m l
i n d e x 0 7 4 1 9 a 3 b 9 2 . . f b 3 d 5 2 f b 1 6 1 0 0 6 4 4
- - - a / d o c / s r c / s g m l / r e f / p s q l - r e f . s g m l
+ + + b / d o c / s r c / s g m l / r e f / p s q l - r e f . s g m l
@ @ - 1 0 3 0 , 1 1 + 1 0 3 0 , 2 9 @ @ I N S E R T I N T O t b l 1 V A L U E S ( $ 1 , $ 2 ) \ b i n d ' f i r s t v a l u e ' ' s e c o n d v a l u e ' \ g
< / v a r l i s t e n t r y >
< v a r l i s t e n t r y i d = " a p p - p s q l - m e t a - c o m m a n d - c o n n i n f o " >
- < t e r m > < l i t e r a l > \ c o n n i n f o < / l i t e r a l > < / t e r m >
+ < t e r m > < l i t e r a l > \ c o n n i n f o [ + ] < / l i t e r a l > < / t e r m >
< l i s t i t e m >
- < p a r a >
- O u t p u t s i n f o r m a t i o n a b o u t t h e c u r r e n t d a t a b a s e c o n n e c t i o n .
- < / p a r a >
+ < p a r a >
+ O u t p u t s a s t r i n g d i s p l a y i n g i n f o r m a t i o n a b o u t t h e c u r r e n t
+ d a t a b a s e c o n n e c t i o n . W h e n < l i t e r a l > + < / l i t e r a l > i s a p p e n d e d ,
+ m o r e d e t a i l s a b o u t t h e c o n n e c t i o n a r e d i s p l a y e d i n t a b l e
+ f o r m a t :
+ < s i m p l e l i s t >
+ < m e m b e r > < l i t e r a l > P r o t o c o l V e r s i o n : < / l i t e r a l > T h e v e r s i o n o f t h e P o s t g r e S Q L p r o t o c o l u s e d f o r
+ t h i s c o n n e c t i o n . < / m e m b e r >
+ < m e m b e r > < l i t e r a l > S S L C o n n e c t i o n : < / l i t e r a l > T r u e i f t h e c u
Re: On disable_cost
On Thu, Aug 1, 2024 at 11:34 PM David Rowley wrote: > I'm not planning on pushing this any further. I've just tried to > highlight that there's the possibility of a behavioural change. You're > claiming there isn't one. I claim there is. I don't know what to tell you. The original version of the patch didn't change this stuff, and the result did not work. So I looked into the problem and fixed it. I may have done that wrongly, or there may be debatable points, but it seems like your argument is essentially that I shouldn't have done any of this and I should just take it all back out, and I know that doesn't work because it's the first thing I tried. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Conflict detection and logging in logical replication
Performance tests done on the v8-0001 and v8-0002 patches, available at [1]. The purpose of the performance tests is to measure the impact on logical replication with track_commit_timestamp enabled, as this involves fetching the commit_ts data to determine delete_differ/update_differ conflicts. Fortunately, we did not see any noticeable overhead from the new commit_ts fetch and comparison logic. The only notable impact is potential overhead from logging conflicts if they occur frequently. Therefore, enabling conflict detection by default seems feasible, and introducing a new detect_conflict option may not be necessary. Please refer to the following for detailed test results: For all the tests, created two server nodes, one publisher and one as subscriber. Both the nodes are configured with below settings - wal_level = logical shared_buffers = 40GB max_worker_processes = 32 max_parallel_maintenance_workers = 24 max_parallel_workers = 32 synchronous_commit = off checkpoint_timeout = 1d max_wal_size = 24GB min_wal_size = 15GB autovacuum = off Test 1: create conflicts on Sub using pgbench. Setup: - Both publisher and subscriber have pgbench tables created as- pgbench -p $node1_port postgres -qis 1 - At Sub, a subscription created for all the changes from Pub node. Test Run: - To test, ran pgbench for 15 minutes on both nodes simultaneously, which led to concurrent updates and update_differ conflicts on the Subscriber node. Command used to run pgbench on both nodes- ./pgbench postgres -p 8833 -c 10 -j 3 -T 300 -P 20 Results: For each case, note the “tps” and total time taken by the apply-worker on Sub to apply the changes coming from Pub. Case1: track_commit_timestamp = off, detect_conflict = off Pub-tps = 9139.556405 Sub-tps = 8456.787967 Time of replicating all the changes: 19min 28s Case 2 : track_commit_timestamp = on, detect_conflict = on Pub-tps = 8833.016548 Sub-tps = 8389.763739 Time of replicating all the changes: 20min 20s Case3: track_commit_timestamp = on, detect_conflict = off Pub-tps = 8886.101726 Sub-tps = 8374.508017 Time of replicating all the changes: 19min 35s Case 4: track_commit_timestamp = off, detect_conflict = on Pub-tps = 8981.924596 Sub-tps = 8411.120808 Time of replicating all the changes: 19min 27s **The difference of TPS between each case is small. While I can see a slight increase of the replication time (about 5%), when enabling both track_commit_timestamp and detect_conflict. Test2: create conflict using a manual script - To measure the precise time taken by the apply-worker in all cases, create a test with a table having 10 million rows. - To record the total time taken by the apply-worker, dump the current time in the logfile for apply_handle_begin() and apply_handle_commit(). Setup: Pub : has a table ‘perf’ with 10 million rows. Sub : has the same table ‘perf’ with its own 10 million rows (inserted by 1000 different transactions). This table is subscribed for all changes from Pub. Test Run: At Pub: run UPDATE on the table ‘perf’ to update all its rows in a single transaction. (this will lead to update_differ conflict for all rows on Sub when enabled). At Sub: record the time(from log file) taken by the apply-worker to apply all updates coming from Pub. Results: Below table shows the total time taken by the apply-worker (apply_handle_commit Time - apply_handle_begin Time ). (Two test runs for each of the four cases) Case1: track_commit_timestamp = off, detect_conflict = off Run1 - 2min 42sec 579ms Run2 - 2min 41sec 75ms Case 2 : track_commit_timestamp = on, detect_conflict = on Run1 - 6min 11sec 602ms Run2 - 6min 25sec 179ms Case3: track_commit_timestamp = on, detect_conflict = off Run1 - 2min 34sec 223ms Run2 - 2min 33sec 482ms Case 4: track_commit_timestamp = off, detect_conflict = on Run1 - 2min 35sec 276ms Run2 - 2min 38sec 745ms ** In the case-2 when both track_commit_timestamp and detect_conflict are enabled, the time taken by the apply-worker is ~140% higher. Test3: Case when no conflict is detected. To measure the time taken by the apply-worker when there is no conflict detected. This test is to confirm if the time overhead in Test1-Case2 is due to the new function GetTupleCommitTs() which fetches the origin and timestamp information for each row in the table before applying the update. Setup: - The Publisher and Subscriber both have an empty table to start with. - At Sub, the table is subscribed for all changes from Pub. - At Pub: Insert 10 million rows and the same will be replicated to the Sub table as well. Test Run: At Pub: run an UPDATE on the table to update all rows in a single transaction. (This will NOT hit the update_differ
Re: On disable_cost
On Sat, 3 Aug 2024 at 00:17, Robert Haas wrote: > > On Thu, Aug 1, 2024 at 11:34 PM David Rowley wrote: > > I'm not planning on pushing this any further. I've just tried to > > highlight that there's the possibility of a behavioural change. You're > > claiming there isn't one. I claim there is. > > I don't know what to tell you. The original version of the patch > didn't change this stuff, and the result did not work. So I looked > into the problem and fixed it. I may have done that wrongly, or there > may be debatable points, but it seems like your argument is > essentially that I shouldn't have done any of this and I should just > take it all back out, and I know that doesn't work because it's the > first thing I tried. I've just read what you wrote again and I now realise something I didn't before. I now think neither of us got it right. I now think what you'd need to do to be aligned to the current behaviour is have initial_cost_nestloop() add the disabled_nodes for the join's subnodes *only* and have final_cost_nestloop() add the additional disabled_nodes if enable_nestloop = off. That way you maintain the existing behaviour of not optimising for disabled node types and don't risk plan changes if the final cost comes out cheaper than the initial cost. David
Re: pg_combinebackup does not detect missing files
On Fri, Apr 19, 2024 at 11:47 AM Robert Haas wrote: > Hmm, that's an interesting perspective. I've always been very > skeptical of doing verification only around missing files and not > anything else. I figured that wouldn't be particularly meaningful, and > that's pretty much the only kind of validation that's even > theoretically possible without a bunch of extra overhead, since we > compute checksums on entire files rather than, say, individual blocks. > And you could really only do it for the final backup in the chain, > because you should end up accessing all of those files, but the same > is not true for the predecessor backups. So it's a very weak form of > verification. > > But I looked into it and I think you're correct that, if you restrict > the scope in the way that you suggest, we can do it without much > additional code, or much additional run-time. The cost is basically > that, instead of only looking for a backup_manifest entry when we > think we can reuse its checksum, we need to do a lookup for every > single file in the final input directory. Then, after processing all > such files, we need to iterate over the hash table one more time and > see what files were never touched. That seems like an acceptably low > cost to me. So, here's a patch. > > I do think there's some chance that this will encourage people to > believe that pg_combinebackup is better at finding problems than it > really is or ever will be, and I also question whether it's right to > keep changing stuff after feature freeze. But I have a feeling most > people here are going to think this is worth including in 17. Let's > see what others say. There was no hue and cry to include this in v17 and I think that ship has sailed at this point, but we could still choose to include this as an enhancement for v18 if people want it. I think David's probably in favor of that (but I'm not 100% sure) and I have mixed feelings about it (explained above) so what I'd really like is some other opinions on whether this idea is good, bad, or indifferent. Here is a rebased version of the patch. No other changes since v1. -- Robert Haas EDB: http://www.enterprisedb.com v2-0001-pg_combinebackup-Detect-missing-files-when-possib.patch Description: Binary data
Re: can we mark upper/lower/textlike functions leakproof?
On Thu, Aug 1, 2024 at 6:03 PM Robert Haas wrote: > > On Thu, Aug 1, 2024 at 4:45 PM Jacob Champion > wrote: > > Would it provide enough value for effort to explicitly mark leaky > > procedures as such? Maybe that could shrink the grey area enough to be > > protective? > > You mean like proleakproof = true/false/maybe? Yeah, exactly. --Jacob
Re: Memory growth observed with C++ application consuming libpq.dll on Windows
Rajesh Kokkonda writes: > Are you looking for a fully functional sample program or only the APIs from > libpq library that our product uses? I am asking this because if the > requirement is to have a sample code, then I will have to work on creating > one on the same lines as our product. Just for the record, the last field-reported memory leak in libpq was found/fixed in 2020, and it occurred only when using GSSAPI encryption. Previous reports weren't frequent either. So while it may be that you've found one, it seems far more likely that the fault is in your application. In any case, nobody is likely to spend time looking for a bug that may not be there unless you can produce a self-contained test case demonstrating a leak. If we had a test case, the first thing we'd likely do would be to run it under Valgrind, to see if automated analysis is enough to locate the logic fault. So an alternative you could consider before trying to extract a test case is to run your app under Valgrind for yourself. As a bonus, that has a decent shot at locating the fault whether it's ours or yours. I'm not sure if Valgrind is available for Windows though --- can you easily put the app on a different platform? regards, tom lane
Re: Remove obsolete RECHECK keyword completely
Peter Eisentraut writes: > I propose to remove the obsolete RECHECK keyword completely. This used > to be part of CREATE OPERATOR CLASS and ALTER OPERATOR FAMILY, but it > has done nothing (except issue a NOTICE) since PostgreSQL 8.4. Commit > 30e7c175b81 removed support for dumping from pre-9.2 servers, so this no > longer serves any need, it seems to me. +1 for the idea; didn't vet the patch closely. regards, tom lane
Re: can we mark upper/lower/textlike functions leakproof?
On 8/2/24 09:48, Jacob Champion wrote: On Thu, Aug 1, 2024 at 6:03 PM Robert Haas wrote: On Thu, Aug 1, 2024 at 4:45 PM Jacob Champion wrote: > Would it provide enough value for effort to explicitly mark leaky > procedures as such? Maybe that could shrink the grey area enough to be > protective? You mean like proleakproof = true/false/maybe? Yeah, exactly. Hmmm, and then have "leakproof_mode" = strict/lax/off where 'strict' is current behavior, 'lax' allows the 'maybe's to get pushed down, and 'off' ignores the leakproof attribute entirely and pushes down anything that merits being pushed? -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Why is citext/regress failing on hamerkop?
On Aug 1, 2024, at 18:54, Thomas Munro wrote: > Done (e52a44b8, 91f498fd). > > Any elucidation on how and why Windows machines have started using > UTF-8 would be welcome. Haven’t been following this thread, but this post reminded me of an issue I saw with locales on Windows[1]. Could it be that the introduction of Universal CRT[2] in Windows 10 has improved UTF-8 support? Bit of a wild guess, but I assume worth bringing up at least. D [1]: https://github.com/shogo82148/actions-setup-perl/issues/1713 [2]: https://learn.microsoft.com/en-us/cpp/porting/upgrade-your-code-to-the-universal-crt?view=msvc-170
Re: can we mark upper/lower/textlike functions leakproof?
On Fri, Aug 2, 2024 at 6:58 AM Joe Conway wrote: > On 8/2/24 09:48, Jacob Champion wrote: > > On Thu, Aug 1, 2024 at 6:03 PM Robert Haas > wrote: > >> > >> On Thu, Aug 1, 2024 at 4:45 PM Jacob Champion > >> wrote: > >> > Would it provide enough value for effort to explicitly mark leaky > >> > procedures as such? Maybe that could shrink the grey area enough to be > >> > protective? > >> > >> You mean like proleakproof = true/false/maybe? > > > > Yeah, exactly. > > > Hmmm, and then have "leakproof_mode" = strict/lax/off where 'strict' is > current behavior, 'lax' allows the 'maybe's to get pushed down, and > 'off' ignores the leakproof attribute entirely and pushes down anything > that merits being pushed? > > > Another approach would be to do something like: select "upper"!(column) to demark within the query usage itself that this function with a maybe leakproof attribute gets interpreted as yes. or even something like: select "upper"[leakproof](column) This is both more readable and provides a way, not that we seem to need it, to place more than one label (or just alternative labels using the same syntax construct, like with explain (...)) inside the "array". Discoverability of when to add such a marker would be left up to the query author, with the safe default mode being a not leakproof interpretation. David J.
Re: Memory growth observed with C++ application consuming libpq.dll on Windows
Okay. I will try to create one sample program and send it to you sometime next week. In the meantime, I am listing down all the methods we are consuming from libpq. PQconnectdbParams PQstatus PQerrorMessage PQpingParams PQfinish PQresultStatus PQclear PQsetSingleRowMode PQntuples PQnfields PQftype PQgetvalue PQgetlength PQgetisnull PQgetCancel PQfreeCancel PQcancel PQsetErrorVerbosity PQsendPrepare PQsendQueryPrepared PQgetResult PQconsumeInput PQisBusy PQsetnonblocking PQflush PQsocket PQtransactionStatus PQresultErrorField Regards, Rajesh On Fri, Aug 2, 2024 at 5:06 PM Yasir wrote: > > On Fri, Aug 2, 2024 at 1:53 PM Rajesh Kokkonda > wrote: > >> Hi Yasir, >> >> Are you looking for a fully functional sample program or only the APIs >> from libpq library that our product uses? I am asking this because if the >> requirement is to have a sample code, then I will have to work on creating >> one on the same lines as our product. >> >> > A functional sample is always best and preferred, however, only APIs used > by your product would also be sufficient. > > Rajesh >> >> On Thu, Aug 1, 2024 at 5:27 PM Yasir >> wrote: >> >>> Hi Rajesh, >>> >>> Can you please attach a sample code snippet showing libpq's functions >>> being called? It will help to identify the libpq's functions to investigate >>> further for a potential mem leak. >>> >>> Regards... >>> >>> Yasir Hussain >>> >>> On Thu, Aug 1, 2024 at 4:30 PM Rajesh Kokkonda < >>> rajeshk.kokko...@gmail.com> wrote: >>> Hi, We are seeing a gradual growth in the memory consumption of our process on Windows. Ours is a C++ application that directly loads libpq.dll and handles the queries and functions. We use setSingleRowMethod to limit the number of rows returned simultaneously to the application. We do not observe any memory increase when the application is run on Linux. There is no code difference between Windows and Linux from the application standpoint. We ran valgrind against our application on Linux and found no memory leaks. Since the same code is being used on Windows as well, we do not suspect any memory leak there. The question is if there are any known memory leaks with the version of the library we are using on Windows. Kindly let us know. The version of the library on Linux is libpq.so.5.16 The windows version of the library is 16.0.3.0 [image: image.png] Thanks, Rajesh >>>
Re: can we mark upper/lower/textlike functions leakproof?
Joe Conway writes: > > Hmmm, and then have "leakproof_mode" = strict/lax/off where 'strict' is > current behavior, 'lax' allows the 'maybe's to get pushed down, and > 'off' ignores the leakproof attribute entirely and pushes down anything > that merits being pushed? > So in other words, we might as well just remove RLS. regards, tom lane
Re: Parallel CREATE INDEX for GIN indexes
On Tue, 9 Jul 2024 at 03:18, Andy Fan wrote: >> and later we called 'tuplesort_performsort(state->bs_sortstate);'. Even >> we have some CTID merges activity in '(1)', the tuples are still >> ordered, so the sort (in both tuplesort_putgintuple and >> 'tuplesort_performsort) are not necessary, what's more, in the each of >> 'flush-memory-to-disk' in tuplesort, it create a 'sorted-run', and in >> this case, acutally we only need 1 run only since all the input tuples >> in the worker is sorted. The reduction of 'sort-runs' in worker will be >> helpful to leader's final mergeruns. the 'sorted-run' benefit doesn't >> exist for the case-1 (RBTree -> worker_state). >> >> If Matthias's proposal is adopted, my optimization will not be useful >> anymore and Matthias's porposal looks like a more natural and effecient >> way. I think they might be complementary. I don't think it's reasonable to expect GIN's BuildAccumulator to buffer all the index tuples at the same time (as I mentioned upthread: we are or should be limited by work memory), but the BuildAccumulator will do a much better job at combining tuples than the in-memory sort + merge-write done by Tuplesort (because BA will use (much?) less memory for the same number of stored values). So, the idea of making BuildAccumulator responsible for providing the initial sorted runs does resonate with me, and can also be worth pursuing. I think it would indeed save time otherwise spent comparing if tuples can be merged before they're first spilled to disk, when we already have knowledge about which tuples are a sorted run. Afterwards, only the phases where we merge sorted runs from disk would require my buffered write approach that merges Gin tuples. Kind regards, Matthias van de Meent Neon (https://neon.tech)
Re: On disable_cost
On Fri, Aug 2, 2024 at 9:13 AM David Rowley wrote: > I now think neither of us got it right. I now think what you'd need to > do to be aligned to the current behaviour is have > initial_cost_nestloop() add the disabled_nodes for the join's subnodes > *only* and have final_cost_nestloop() add the additional > disabled_nodes if enable_nestloop = off. That way you maintain the > existing behaviour of not optimising for disabled node types and don't > risk plan changes if the final cost comes out cheaper than the initial > cost. All three initial_cost_XXX functions have a comment that says "This must quickly produce lower-bound estimates of the path's startup and total costs," i.e. the final cost should never be cheaper. I'm pretty sure that it was the design intention here that no path ever gets rejected at the initial cost stage that would have been accepted at the final cost stage. (You can also see, as a matter of implementation, that they extract the startup_cost and run_cost from the workspace and then add to those values.) -- Robert Haas EDB: http://www.enterprisedb.com
Re: wrong translation file reference in pg_createsubscriber
Hello, On 2024-Aug-02, Kyotaro Horiguchi wrote: > I found that pg_createsubscriber doesn't use NLS files. This is due to > the wrong reference name "pg_createsubscriber" being passed to > set_pglocale_pgservice(). It should be "pg_basebackup" instead. See > the attached patch. Absolutely right. Pushed, thanks. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: can we mark upper/lower/textlike functions leakproof?
On 8/2/24 11:07, Tom Lane wrote: Joe Conway writes: Hmmm, and then have "leakproof_mode" = strict/lax/off where 'strict' is current behavior, 'lax' allows the 'maybe's to get pushed down, and 'off' ignores the leakproof attribute entirely and pushes down anything that merits being pushed? So in other words, we might as well just remove RLS. Perhaps deciding where to draw the line for 'maybe' is too hard, but I don't understand how you can say that in a general sense. 'strict' mode would provide the same guarantees as today. And even 'off' has utility for cases where (1) no logins are allowed except for the app (which is quite common in production environments) and no database errors are propagated to the end client (again pretty standard best practice); or (2) non-production environments, e.g. for testing or debugging; or (3) use cases that utilize RLS as a notationally convenient filtering mechanism and are not bothered by some leakage in the worst case. -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Memory growth observed with C++ application consuming libpq.dll on Windows
Em sex., 2 de ago. de 2024 às 11:54, Rajesh Kokkonda < rajeshk.kokko...@gmail.com> escreveu: > Okay. I will try to create one sample program and send it to you sometime > next week. In the meantime, I am listing down all the methods we are > consuming from libpq. > > PQconnectdbParams > PQstatus > PQerrorMessage > PQpingParams > PQfinish > PQresultStatus > PQclear > PQsetSingleRowMode > PQntuples > PQnfields > PQftype > PQgetvalue > PQgetlength > PQgetisnull > PQgetCancel > PQfreeCancel > PQcancel > PQsetErrorVerbosity > PQsendPrepare > PQsendQueryPrepared > PQgetResult > PQconsumeInput > PQisBusy > PQsetnonblocking > PQflush > PQsocket > PQtransactionStatus > PQresultErrorField > > It is highly likely that the memory consumption is caused by your application. Perhaps due to the lack of freeing up the resources used by the library. You can try using this tool, to find out the root cause. https://drmemory.org/ best regards, Ranier Vilela
Re: can we mark upper/lower/textlike functions leakproof?
On Fri, Aug 2, 2024 at 9:13 AM Joe Conway wrote: > > On 8/2/24 11:07, Tom Lane wrote: > > Joe Conway writes: > >> > >> Hmmm, and then have "leakproof_mode" = strict/lax/off where 'strict' is > >> current behavior, 'lax' allows the 'maybe's to get pushed down, and > >> 'off' ignores the leakproof attribute entirely and pushes down anything > >> that merits being pushed? > >> > > > > So in other words, we might as well just remove RLS. > > Perhaps deciding where to draw the line for 'maybe' is too hard, but I > don't understand how you can say that in a general sense. I'm more sympathetic to the "maybe" case, but for the "off" proposal I find myself agreeing with Tom. If you want "off", turn off RLS. > And even 'off' > has utility for cases where (1) no logins are allowed except for the app > (which is quite common in production environments) and no database > errors are propagated to the end client (again pretty standard best > practice); I'm extremely skeptical of this statement, but I've been out of the full-stack world for a bit. How does a modern best-practice application hide the fact that it ran into an error and couldn't complete a query? > or (2) non-production environments, e.g. for testing or > debugging; or Changing the execution behavior between dev and prod seems like an anti-goal. When would turning this off help you debug something? > (3) use cases that utilize RLS as a notationally > convenient filtering mechanism and are not bothered by some leakage in > the worst case. Catering to this use case doesn't seem like it should be a priority. If a security feature is useful for you in a non-security setting, awesome, but we shouldn't poke holes in it for that situation, nor should it be surprising if the security gets stronger and becomes harder to use for non-security. --Jacob
Re: can we mark upper/lower/textlike functions leakproof?
On Fri, Aug 2, 2024 at 11:07 AM Tom Lane wrote: > Joe Conway writes: > > > > Hmmm, and then have "leakproof_mode" = strict/lax/off where 'strict' is > > current behavior, 'lax' allows the 'maybe's to get pushed down, and > > 'off' ignores the leakproof attribute entirely and pushes down anything > > that merits being pushed? > > > > So in other words, we might as well just remove RLS. Hey, everybody, I don't think Tom likes the proposal. I'll be honest: I don't like it, either. I don't even like proleakproof=true/false/maybe; I asked about that to understand if that was what Jacob was proposing, not because I actually think we should do it. The problem is that there's likely to be a fairly wide range contained inside of "maybe", with cases like "upper" at the safer end of the spectrum. That's too fuzzy to use as a basis for any sort of real security, IMHO; we won't be able to find two hackers who agree on how anything should be marked. I think part of our problem here is that we have very few examples of how to actually analyze a function for leakproof-ness, or how to exploit one that is erroneously so marked. The conversations then tend to degenerate into some people saying things are scary and some people saying the scariness is overrated and then the whole thing just becomes untethered from reality. Maybe we need to create some really robust documentation in this area so that we can move toward a common conceptual framework, instead of everybody just having a lot of opinions. I can't shake the feeling that if PostgreSQL got the same level of attention from security researchers that Linux or OpenSSL do, this would be a very different conversation. The fact that we have more people complaining about RLS causing poor query performance than we do about RLS leaking information is probably a sign that it's being used to provide more security theatre than actual security. Even the leaks we intended to have are pretty significant, and I'm sure that we have some we didn't intend. -- Robert Haas EDB: http://www.enterprisedb.com
Re: can we mark upper/lower/textlike functions leakproof?
On Fri, Aug 2, 2024 at 9:22 AM Robert Haas wrote: > I'll be honest: I don't like it, either. I don't even like > proleakproof=true/false/maybe; I asked about that to understand if > that was what Jacob was proposing, not because I actually think we > should do it. The problem is that there's likely to be a fairly wide > range contained inside of "maybe", with cases like "upper" at the > safer end of the spectrum. That's too fuzzy to use as a basis for any > sort of real security, IMHO; we won't be able to find two hackers who > agree on how anything should be marked. I guess I wasn't trying to propose that the grey area be used as the basis for security, but that we establish a lower bound for the grey. Make things strictly better than today, and cut down on the fear that someone's going to accidentally mark something that we all agree shouldn't be. And then shrink the grey area over time as we debate. (Now, if there aren't that many cases where we can all agree on "unsafe", then the proposal loses pretty much all value, because we'll never shrink the uncertainty.) > I think part of our problem here is that we have very few examples of > how to actually analyze a function for leakproof-ness, or how to > exploit one that is erroneously so marked. The conversations then tend > to degenerate into some people saying things are scary and some people > saying the scariness is overrated and then the whole thing just > becomes untethered from reality. Maybe we need to create some really > robust documentation in this area so that we can move toward a common > conceptual framework, instead of everybody just having a lot of > opinions. +1 --Jacob
Re: Memory growth observed with C++ application consuming libpq.dll on Windows
We did run our application under valgrind on Linux. We did not see any leaks. There is no platform dependent code in our application. We are seeing gradual memory growth only on windows. That is what lead me to believe the leak may be present in postgresql. I will run under available memory tools on windows and get back to you. I will also try to create a sample and see if I can reproduce the problem. Thanks, Rajesh On Fri, 2 Aug 2024, 21:45 Ranier Vilela, wrote: > Em sex., 2 de ago. de 2024 às 11:54, Rajesh Kokkonda < > rajeshk.kokko...@gmail.com> escreveu: > >> Okay. I will try to create one sample program and send it to you sometime >> next week. In the meantime, I am listing down all the methods we are >> consuming from libpq. >> >> PQconnectdbParams >> PQstatus >> PQerrorMessage >> PQpingParams >> PQfinish >> PQresultStatus >> PQclear >> PQsetSingleRowMode >> PQntuples >> PQnfields >> PQftype >> PQgetvalue >> PQgetlength >> PQgetisnull >> PQgetCancel >> PQfreeCancel >> PQcancel >> PQsetErrorVerbosity >> PQsendPrepare >> PQsendQueryPrepared >> PQgetResult >> PQconsumeInput >> PQisBusy >> PQsetnonblocking >> PQflush >> PQsocket >> PQtransactionStatus >> PQresultErrorField >> >> It is highly likely that the memory consumption is caused by your > application. > Perhaps due to the lack of freeing up the resources used by the library. > You can try using this tool, to find out the root cause. > > https://drmemory.org/ > > best regards, > Ranier Vilela >
Re: On disable_cost
Robert Haas writes: > On Fri, Aug 2, 2024 at 9:13 AM David Rowley wrote: >> ... That way you maintain the >> existing behaviour of not optimising for disabled node types and don't >> risk plan changes if the final cost comes out cheaper than the initial >> cost. > All three initial_cost_XXX functions have a comment that says "This > must quickly produce lower-bound estimates of the path's startup and > total costs," i.e. the final cost should never be cheaper. I'm pretty > sure that it was the design intention here that no path ever gets > rejected at the initial cost stage that would have been accepted at > the final cost stage. That absolutely is the expectation, and we'd better be careful not to break it. regards, tom lane
Re: On disable_cost
On Fri, Aug 2, 2024 at 12:51 PM Tom Lane wrote: > That absolutely is the expectation, and we'd better be careful not > to break it. I have every intention of not breaking it. :-) -- Robert Haas EDB: http://www.enterprisedb.com
Re: Injection points: preloading and runtime arguments
> On 18 Jul 2024, at 03:55, Michael Paquier wrote:
>
> If there is anything else you would like to see adjusted in this area,
> please let me know.
I’ve tried to switch my multixact test to new INJECTION_POINT_CACHED… and it
does not work for me. Could you please take a look?
2024-08-02 18:52:32.244 MSK [53155] 001_multixact.pl LOG: statement: select
test_create_multixact();
TRAP: failed Assert("CritSectionCount == 0 || (context)->allowInCritSection"),
File: "mcxt.c", Line: 1186, PID: 53155
0 postgres0x0001031212f0 ExceptionalCondition
+ 236
1 postgres0x00010317a01c MemoryContextAlloc +
240
2 postgres0x000102e66158
dsm_create_descriptor + 80
3 postgres0x000102e66474 dsm_attach + 416
4 postgres0x00010316c264 dsa_attach + 24
5 postgres0x000102e69994 init_dsm_registry +
256
6 postgres0x000102e6965c GetNamedDSMSegment +
492
7 injection_points.dylib 0x00010388f2cc injection_init_shmem
+ 68
8 injection_points.dylib 0x00010388efbc injection_wait + 72
9 postgres0x0001031606bc InjectionPointCached
+ 72
10 postgres0x0001028ffc70
MultiXactIdCreateFromMembers + 360
11 postgres0x0001028ffac8 MultiXactIdCreate +
344
12 test_slru.dylib 0x00010376fa04
test_create_multixact + 52
The test works fine with SQL interface “select
injection_points_load('get-new-multixact-id');”.
Thanks!
Best regards, Andrey Borodin.
vAug2-0001-Add-multixact-CV-sleep-test.patch
Description: Binary data
Re: [BUG?] check_exclusion_or_unique_constraint false negative
Hello, Amit!
> I think it is rather less likely or not possible in a parallel apply
> case because such conflicting updates (updates on the same tuple)
> should be serialized at the publisher itself. So one of the updates
> will be after the commit that has the second update.
Glad to hear! But anyway, such logic looks very fragile to me.
> I haven't tried the test based on your description of the general
> problem with DirtySnapshot scan. In case of logical replication, we
> will LOG update_missing type of conflict and the user may need to take
> some manual action based on that.
Current it is just DEBUG1, so it will be probably missed by the user.
> * XXX should this be promoted to ereport(LOG) perhaps?
> */
> elog(DEBUG1,
> "logical replication did not find row to be updated "
> "in replication target relation \"%s\"",
> RelationGetRelationName(localrel));
> }
> I have not tried a test so I could
> be wrong as well. I am not sure we can do anything specific to logical
> replication for this but feel free to suggest if you have ideas to
> solve this problem in general or specific to logical replication.
I've implemented a solution to address the problem more generally, attached
the patch (and also the link [1]).
Here's a summary of the changes:
* For each tuple skipped because it was deleted, we now accumulate the
maximum xmax.
* Before the scan begins, we store the value of the latest completed
transaction.
* If no tuples are found in the index, we check the max(xmax) value. If
this value is newer than the latest completed transaction stored before the
scan, it indicates that a tuple was deleted by another transaction after
the scan started. To ensure all tuples are correctly processed we then
rescan the index.
Also added a test case to cover this scenario using the new injection point
mechanism and
updated the b-tree index documentation to include a description of this
case.
I'll add this into the next commitfest.
Best regards,
Mikhail.
[1]:
https://github.com/postgres/postgres/compare/master...michail-nikolaev:postgres:concurrent_unique
From b639379c393c70ca322bab57222513e71c96ad78 Mon Sep 17 00:00:00 2001
From: nkey
Date: Fri, 2 Aug 2024 16:20:32 +0200
Subject: [PATCH v1] fix for lost record in case of DirtySnapshot index scans +
docs + test
---
contrib/pgstattuple/pgstattuple.c | 2 +-
src/backend/access/heap/heapam_handler.c | 2 +-
src/backend/access/heap/heapam_visibility.c | 20 +++-
src/backend/access/nbtree/README | 10
src/backend/access/nbtree/nbtinsert.c | 2 +-
src/backend/access/transam/varsup.c | 11 +
src/backend/executor/execIndexing.c | 29 +++-
src/backend/executor/execReplication.c| 26 +-
src/backend/replication/logical/origin.c | 2 +-
src/include/access/transam.h | 16 +++
src/include/utils/snapmgr.h | 8 +++-
src/include/utils/snapshot.h | 5 +-
.../test_misc/t/006_dirty_index_scan.pl | 47 +++
13 files changed, 168 insertions(+), 12 deletions(-)
create mode 100644 src/test/modules/test_misc/t/006_dirty_index_scan.pl
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 3bd8b96197..4d1b469d8e 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -332,7 +332,7 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo)
scan = table_beginscan_strat(rel, SnapshotAny, 0, NULL, true, false);
hscan = (HeapScanDesc) scan;
- InitDirtySnapshot(SnapshotDirty);
+ InitDirtySnapshot(SnapshotDirty, NULL);
nblocks = hscan->rs_nblocks; /* # blocks to be scanned */
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 6f8b1b7929..7ea4b205d5 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -404,7 +404,7 @@ tuple_lock_retry:
*
* Loop here to deal with updated or busy tuples
*/
- InitDirtySnapshot(SnapshotDirty);
+ InitDirtySnapshot(SnapshotDirty, NULL);
for (;;)
{
if (ItemPointerIndicatesMovedPartitions(tid))
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 9243feed01..91de5dcea1 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -719,6 +719,12 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
return TM_Deleted; /* deleted by other */
}
+inline static void UpdateDirtyMaxXmax(Snapshot snapshot, TransactionId xmax)
+{
+ if (snapshot->xip != NULL)
+ snapshot->xip[0] = TransactionIdNewer(xmax, snapshot->xip[0]);
+}
+
/*
* HeapTupleSatisfiesDirty
* True iff heap tuple is valid including effects of open transactions.
@@ -737,7 +743,9 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
* Similarly for snapshot->xmax and the tuple's xmax. I
Re: [PoC] Federated Authn/z with OAUTHBEARER
On 30.07.24 00:30, Jacob Champion wrote: But under what circumstances does "the linker doesn't strip out" happen? If this happens accidentally, then we should have seen some buildfarm failures or something? On my machine, for example, I see differences with optimization levels. Say you inadvertently call pfree() in a _shlib build, as I did multiple times upthread. By itself, that shouldn't actually be a problem (it eventually redirects to free()), so it should be legal to call pfree(), and with -O2 the build succeeds. But with -Og, the exit() check trips, and when I disassemble I see that pg_malloc() et all have infected the shared object. After all, we did tell the linker to put that object file in, and we don't ask it to garbage-collect sections. I'm tempted to say, this is working as intended. libpgcommon is built as a static library. So we can put all the object files in the library, and its users only use the object files they really need. So this garbage collection you allude to actually does happen, on an object-file level. You shouldn't use pfree() interchangeably with free(), even if that is not enforced because it's the same thing underneath. First, it just makes sense to keep the alloc and free pairs matched up. And second, on Windows there is some additional restriction (vague knowledge) that the allocate and free functions must be in the same library, so mixing them freely might not even work.
Re: New compiler warnings in buildfarm
On 31.07.24 20:39, Andres Freund wrote: On 2024-07-31 11:32:56 -0700, Andres Freund wrote: On 2024-07-31 10:11:07 -0400, Tom Lane wrote: It looks like serinus needs this fix too. Added to both. I've forced runs for both animals, so the bf should show results of that soon. I Wonder if I should also should add -Wno-clobbered to serinus' config. Afaict -Wclobbered is pretty useless once optimizations are used. I've long added that to my local dev environment flags because it's so noisy (which is too bad, in theory a good warning for this would be quite helpful). It's unclear to me what to make of this. We have in the past fixed a number of these, IIRC, and clearly in theory the risk that the warning points out does exist. But these warnings appear erratic and inconsistent. I'm using the same compiler versions but I don't see any of these warnings. So I don't understand exactly what triggers these.
Re: can we mark upper/lower/textlike functions leakproof?
On Fri, Aug 2, 2024 at 12:33 PM Jacob Champion
wrote:
> (Now, if there aren't that many cases where we can all agree on
> "unsafe", then the proposal loses pretty much all value, because we'll
> never shrink the uncertainty.)
My belief is that nearly everything in unsafe. We ship with very
little marked leakproof right now, and that might be too conservative,
but probably not by much. For example:
-- +(int4, int4) is not leakproof
robert.haas=# select 2147483647::int4+1;
ERROR: integer out of range
-- textcat is not leakproof
robert.haas=# create temp table x as select repeat('a',(2^29)::int4-2)::text a;
SELECT 1
robert.haas=# select length(a||a) from x;
ERROR: invalid memory alloc request size 1073741824
-- absolute value is not leakproof
robert.haas=# select @(-2147483648);
ERROR: integer out of range
-- textlike is not leakproof
robert.haas=# select 'a' ~ 'b\';
ERROR: invalid regular expression: invalid escape \ sequence
-- division is not leakproof
robert.haas=# select 2/0;
ERROR: division by zero
-- to_date is not leakproof
robert.haas=# select to_date('abc', 'def');
ERROR: invalid value "a" for "d"
DETAIL: Value must be an integer.
I think it would be safe to mark the bitwise xor operator for integers
as leakproof. But that isn't likely to be used in a query. Most of the
stuff that people actually use in queries isn't even arguably
leakproof.
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: [PoC] Federated Authn/z with OAUTHBEARER
On Fri, Aug 2, 2024 at 10:13 AM Peter Eisentraut wrote: > You shouldn't use pfree() interchangeably with free(), even if that is > not enforced because it's the same thing underneath. First, it just > makes sense to keep the alloc and free pairs matched up. And second, on > Windows there is some additional restriction (vague knowledge) that the > allocate and free functions must be in the same library, so mixing them > freely might not even work. Ah, I forgot about the CRT problems on Windows. So my statement of "the linker might not garbage collect" is pretty much irrelevant. But it sounds like we agree that we shouldn't be using fe_memutils at all in shlib builds. (If you can't use palloc -- it calls exit -- then you can't use pfree either.) Is 0002 still worth pursuing, once I've correctly wordsmithed the commit? Or did I misunderstand your point? Thanks! --Jacob
Re: can we mark upper/lower/textlike functions leakproof?
On Fri, Aug 2, 2024 at 10:40 AM Robert Haas wrote: > My belief is that nearly everything in unsafe. We ship with very > little marked leakproof right now, and that might be too conservative, > but probably not by much. Then to me, that seems like the best-case scenario for a "maybe" classification. I'm still not sold on the idea of automatically treating all "maybes" as leakproof (personally I'd prefer that users surgically opt in), but if the pool is small... Thanks, --Jacob
Re: Official devcontainer config
On 01.08.24 23:38, Andrew Dunstan wrote: Not totally opposed, and I will probably give it a try very soon, but I'm wondering if this really needs to go in the core repo. We've generally shied away from doing much in the way of editor / devenv support, trying to be fairly agnostic. It's true we carry .dir-locals.el and .editorconfig, so that's not entirely true, but those are really just about supporting our indentation etc. standards. Yeah, the editor support in the tree ought to be minimal and factual, based on coding standards and widely recognized best practices, not a collection of one person's favorite aliases and scripts. If the scripts are good, let's look at them and maybe put them under src/tools/ for everyone to use. But a lot of this looks like it will requite active maintenance if output formats or node formats or build targets etc. change. And other things require specific local paths. That's fine for a local script or something, but not for a mainline tool that the community will need to maintain. I suggest to start with a very minimal configuration. What are the settings that absolute everyone will need, maybe to set indentation style or something.
Re: [PoC] Federated Authn/z with OAUTHBEARER
On 02.08.24 19:51, Jacob Champion wrote: But it sounds like we agree that we shouldn't be using fe_memutils at all in shlib builds. (If you can't use palloc -- it calls exit -- then you can't use pfree either.) Is 0002 still worth pursuing, once I've correctly wordsmithed the commit? Or did I misunderstand your point? Yes, I think with an adjusted comment and commit message, the actual change makes sense.
Re: New compiler warnings in buildfarm
Peter Eisentraut writes:
> On 31.07.24 20:39, Andres Freund wrote:
>> I Wonder if I should also should add -Wno-clobbered to serinus' config.
>> Afaict
>> -Wclobbered is pretty useless once optimizations are used. I've long added
>> that to my local dev environment flags because it's so noisy (which is too
>> bad, in theory a good warning for this would be quite helpful).
> It's unclear to me what to make of this. We have in the past fixed a
> number of these, IIRC, and clearly in theory the risk that the warning
> points out does exist. But these warnings appear erratic and
> inconsistent. I'm using the same compiler versions but I don't see any
> of these warnings. So I don't understand exactly what triggers these.
Yeah, -Wclobbered's results seem to vary quite a lot across different
compiler versions and perhaps different compiler options. I'd be more
excited about trying to silence it if there were some consistency to
the reports, but there's not that much; plus, we've never seen any
evidence that the reports from the noisier compilers correspond to
real bugs.
Just for context, here's a quick count of -Wclobbered warnings in
the buildfarm:
71 calliphoridae
66 canebrake
71 culicidae
67 grassquit
65 serinus
89 skink
66 taipan
68 tamandua
The other hundred-plus animals report zero such warnings.
I also tried slicing the data by the variable being complained of:
$ grep 'Wclobbered' currentwarnings | sed -e 's/.*: argument //' -e 's/.*:
variable //' | awk '{print $1}' | sort | uniq -c
118 '_do_rethrow'
24 '_do_rethrow2'
8 'arrayname'
6 'bump_level'
1 'cell__state'
7 'commandCollected'
8 'commands'
3 'cstr'
6 'cur_datname'
6 'cur_nspname'
6 'cur_relname'
7 'data'
2 'dboid'
8 'dbstrategy'
8 'elevel'
14 'error'
1 'fd'
8 'found_concurrent_worker'
4 'ft_htab'
8 'has_pending_wal'
8 'ib'
8 'import_collate'
8 'import_default'
8 'import_generated'
8 'import_not_null'
1 'is_program'
1 'iter'
8 'loop_body'
8 'method'
6 'named_arg_strings'
7 'nulls'
5 'objname'
1 'options'
7 'params'
8 'primary'
8 'processed'
8 'rel'
8 'relations'
8 'relids_logged'
8 'reltuples'
44 'result'
8 'retry'
17 'retval'
16 'rv'
6 'seq_relids'
8 'sqlstate'
8 'stats'
3 'success'
8 'switch_lsn'
8 'switch_to_superuser'
8 'sync_slotname'
5 'tab'
7 'table_oids'
8 'tb'
6 'update_failover'
2 'update_tuple'
8 'update_two_phase'
8 'vob'
That shows that the apparent similarity of the total number of reports
per animal is illusory: there are some that all eight animals agree
on, but a lot where they don't.
regards, tom lane
Fix memory counter update in reorderbuffer
Hi,
I found a bug in the memory counter update in reorderbuffer. It was
introduced by commit 5bec1d6bc5e, so pg17 and master are affected.
In ReorderBufferCleanupTXN() we zero the transaction size and then
free the transaction entry as follows:
/* Update the memory counter */
ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
/* deallocate */
ReorderBufferReturnTXN(rb, txn);
However, if the transaction entry has toast changes we free them in
ReorderBufferToastReset() called from ReorderBufferToastReset(), and
end up subtracting the memory usage from zero. Which results in an
assertion failure:
TRAP: failed Assert("(rb->size >= sz) && (txn->size >= sz)"), File:
"reorderbuffer.c"
This can happen for example if an error occurs while replaying
transaction changes including toast changes.
I've attached the patch that fixes the problem and includes a test
case (the test part might not be committed as it slows the test case).
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
fix_memory_counter_update_in_reorderbuffer.patch
Description: Binary data
Re: meson "experimental"?
On Thu, May 30, 2024 at 01:32:18PM +0300, Aleksander Alekseev wrote: > > > [*] What is the correct name for this? > > > > I believe in this section it should be "Visual Studio" as we specify > > elsewhere [1][2]. In [2] we name specific required versions. Maybe we > > should reference this section. > > > > While on it, in [2] section 17.7.5 is named "Visual". I don't think > > such a product exists and find it confusing. Maybe we should rename > > the section to "Visual Studio". Agreed. I've pushed your patch for that: > Here are corresponding patches. The ID is new in v17, so I also renamed it like s/installation-notes-visual/installation-notes-visual-studio/. (I didn't examine or push your other patch, which was about $SUBJECT.)
Re: pg_dump: optimize dumpFunc()
On Fri, Aug 02, 2024 at 01:33:45AM -0400, Tom Lane wrote: > I'm a bit concerned about this on two grounds: > > 1. Is it a win for DBs with not so many functions? > > 2. On the other end of the scale, if you've got a *boatload* of > functions, what does it do to pg_dump's memory requirements? > I'm recalling my days at Salesforce, where they had quite a few > thousand pl/pgsql functions totalling very many megabytes of source > text. (Don't recall precise numbers offhand, and they'd be obsolete > by now even if I did.) > > I'm not sure that the results you're showing justify taking any > risk here. Hm. I think this is sufficient reason to withdraw this patch on the spot. -- nathan
Draft release notes for next week's releases are up
See https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ee219ee8c40d88e7a0ef52c3c1b76c90bbd0d164 As usual, please send corrections/comments by Sunday. regards, tom lane
Re: Why is citext/regress failing on hamerkop?
On Sat, Aug 3, 2024 at 2:11 AM David E. Wheeler wrote: > Haven’t been following this thread, but this post reminded me of an issue I > saw with locales on Windows[1]. Could it be that the introduction of > Universal CRT[2] in Windows 10 has improved UTF-8 support? Yeah. We have a few places that claim that Windows APIs can't do UTF-8 and they have to do extra wchar_t conversions, but that doesn't seem to be true on modern Windows. Example: https://github.com/postgres/postgres/blob/7926a9a80f6daf0fcc1feb1bee5c51fd001bc173/src/backend/utils/adt/pg_locale.c#L1814 I suspect that at least when the locale name is "en-US.UTF-8", then the regular POSIXoid strcoll_l() function should just work™ and we could delete all that stuff and save Windows users a lot of wasted CPU cycles.
Re: [HACKERS] make async slave to wait for lsn to be replayed
I noticed the commit and had a question and a comment. There is a small problem in func.sgml in the sentence "After that the changes made of primary". Should be "on primary". In the for loop in WaitForLSNReplay, shouldn't the check for in-recovery be moved up above the call to GetXLogReplayRecPtr? If we get promoted while waiting for the timeout we could call GetXLogReplayRecPtr while not in recovery. Thanks, Kevin.
Re: [HACKERS] make async slave to wait for lsn to be replayed
On Sat, Aug 3, 2024 at 3:45 AM Kevin Hale Boyes wrote: > I noticed the commit and had a question and a comment. > There is a small problem in func.sgml in the sentence "After that the changes > made of primary". Should be "on primary". Thank you for spotting this. Will fix. > In the for loop in WaitForLSNReplay, shouldn't the check for in-recovery be > moved up above the call to GetXLogReplayRecPtr? > If we get promoted while waiting for the timeout we could call > GetXLogReplayRecPtr while not in recovery. This is intentional. After standby gets promoted, GetXLogReplayRecPtr() returns the last WAL position being replayed while being standby. So, if standby reached target lsn before being promoted, we don't have to throw an error. But this gave me an idea that before the loop we probably need to put RecoveryInProgress() check after GetXLogReplayRecPtr() too. I'll recheck that. -- Regards, Alexander Korotkov Supabase
Re: strange context message in spi.c?
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: tested, failed Spec compliant: tested, failed Documentation:tested, failed As tree is branched out for PG17, I guess now it's time to commit. - No need to rebase - make, make-check , install-check verified The new status of this patch is: Ready for Committer
