Fix typo plgsql to plpgsql.
Hi, all Found several typos like plgsql, I think it should be plpgsql. Regards, Zhang Mingli 0001-Fix-typo-plgsql-to-plpgsql.patch Description: Binary data
Fix condition in shm_toc and remove unused function shm_toc_freespace.
Hi, hackers
Some conditions in shm_toc_insert and shm_toc_allocate are bogus, like:
if (toc_bytes + nbytes > total_bytes || toc_bytes + nbytes < toc_bytes)
Remove the condition `toc_bytes + nbytes < toc_bytes` and take a
sizeof(shm_entry) into account in shm_toc_allocate though
shm_toc_allocate does that too.
/* Check for memory exhaustion and overflow. */
- if (toc_bytes + nbytes > total_bytes || toc_bytes + nbytes <
toc_bytes)
+ if (toc_bytes + sizeof(shm_toc_entry) + nbytes > total_bytes)
{
SpinLockRelease(&toc->toc_mutex);
shm_toc_freespace is introduced with shm_toc by original commit 6ddd5137b2, but
is not used since then, so remove it.
Regards,
Zhang Mingli
v0-0001-Fix-condition-in-shm_toc-and-remove-unused-functi.patch
Description: Binary data
Re: Fix condition in shm_toc and remove unused function shm_toc_freespace.
Hi, On Jan 12, 2023, 14:34 +0800, Zhang Mingli , wrote: > Hi, hackers > > Some conditions in shm_toc_insert and shm_toc_allocate are bogus, like: > if (toc_bytes + nbytes > total_bytes || toc_bytes + nbytes < > toc_bytes) > Remove the condition `toc_bytes + nbytes < toc_bytes` and take a > sizeof(shm_entry) into account in shm_toc_allocate though > shm_toc_allocate does that too. shm_toc_insert does that too, and we can report error earlier. Regards, Zhang Mingli
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
HI,
On Dec 27, 2022, 19:02 +0800, Melih Mutlu , wrote:
Hi,
Having FORCE_NULL(*) and FORCE_NOT_NULL(*) sounds good, since postgres already
has FORCE_QUOTE(*).
I just quickly tried out your patch. It worked for me as expected.
One little suggestion:
+ if (cstate->opts.force_notnull_all)
+ {
+ int i;
+ for(i = 0; i < num_phys_attrs; i++)
+ cstate->opts.force_notnull_flags[i] = true;
+ }
Instead of setting force_null/force_notnull flags for all columns, what about
simply setting "attnums" list to cstate->attnumlist?
Something like the following should be enough :
if (cstate->opts.force_null_all)
attnums = cstate->attnumlist;
else
attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_null);
Tanks very much for review.
I got your point and we have to handle the case that there are no force_*
options at all.
So the codes will be like:
```
List *attnums = NIL;
if (cstate->opts.force_notnull_all)
attnums = cstate->attnumlist;
else if (cstate->opts.force_notnull)
attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_notnull);
if (attnums != NIL)
{
// process force_notnull columns
attnums = NIL; // to process other options later
}
if (cstate->opts.force_null_all)
attnums = cstate->attnumlist;
else if (cstate->opts.force_null)
attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_null);
if (attnums != NIL)
{
// process force_null columns
attnums = NIL; // to process other options later
}
```
That seems a little odd.
Or, we could keep attnums as local variables, then the codes will be like:
```
if (cstate->opts.force_notnull_all || cstate->opts.force_notnull)
{
if (cstate->opts.force_notnull_all)
attnums = cstate->attnumlist;
else
attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_notnull);
// process force_notnull columns
}
```
Any other suggestions?
Regards,
Zhang Mingli
Re: Fix condition in shm_toc and remove unused function shm_toc_freespace.
Hi, Regards, Zhang Mingli On Jan 12, 2023, 16:54 +0800, Richard Guo , wrote: > > On Thu, Jan 12, 2023 at 2:50 PM Zhang Mingli wrote: > > On Jan 12, 2023, 14:34 +0800, Zhang Mingli , wrote: > > > Some conditions in shm_toc_insert and shm_toc_allocate are bogus, like: > > > if (toc_bytes + nbytes > total_bytes || toc_bytes + nbytes < > > > toc_bytes)Remove the condition `toc_bytes + nbytes < toc_bytes` and take > > > a sizeof(shm_entry) into account in shm_toc_allocate though > > > shm_toc_allocate does that too. > > shm_toc_insert does that too, and we can report error earlier. > > I don't think we should consider sizeof(shm_toc_entry) in the 'if' > condition in shm_toc_allocate, as this function is not in charge of > allocating a new TOC entry. That's what shm_toc_insert does. Thanks for review. Make sense. Even reserve a sizeof(shm_toc_entry) when shm_toc_allocate, it cloud happen that there is no space when shm_toc_insert in case of other processes may take space after that. Patch updated. v1-0001-Fix-condition-in-shm_toc-and-remove-unused-functi.patch Description: Binary data
Code review in dsa.c
Hi, hackers Found some functions in dsa.c are not used anymore. dsa_create dsa_attach dsa_get_handle dsa_trim dsa_dump We once used dsa_create to create DSA and it ’s all replaced by dsa_create_in_place since commit 31ae1638ce. dsa_attach and dsa_get_handle cooperate with dsa_create. dsa_trim and dsa_dump are introduced by DSA original commit 13df76a537 , but not used since then. So, they are all dead codes, provide a patch to remove them. Regards, Zhang Mingli v0-0001-Remove-unused-functions-in-dsa.c.patch Description: Binary data
Re: Code review in dsa.c
HI, On Jan 15, 2023, 23:43 +0800, Zhang Mingli , wrote: > Hi, hackers > > Found some functions in dsa.c are not used anymore. > > dsa_create > dsa_attach > dsa_get_handle > dsa_trim > dsa_dump > > We once used dsa_create to create DSA and it ’s all replaced by > dsa_create_in_place since commit 31ae1638ce. > dsa_attach and dsa_get_handle cooperate with dsa_create. > dsa_trim and dsa_dump are introduced by DSA original commit 13df76a537 , but > not used since then. > > So, they are all dead codes, provide a patch to remove them. Patch updated. Forget to remove dsa_unpin in dsa.h, dsa_unpin is also not used since commit 13df76a537. The gemel function dsa_pin is only used in pg_stat. Seems reasonable that we don’t need to call dsa_unpin in pg_stat. Regards, Zhang Mingli v1-0001-Remove-unused-functions-in-dsa.c.patch Description: Binary data
Re: Code review in dsa.c
HI, On Jan 16, 2023, 00:10 +0800, Nathan Bossart , wrote: > On Mon, Jan 16, 2023 at 12:04:56AM +0800, Zhang Mingli wrote: > > So, they are all dead codes, provide a patch to remove them. > > I am proposing a new use of dsa_create, dsa_attach, and dsa_get_handle in > https://commitfest.postgresql.org/41/4020/. These might also be useful for > extensions, so IMHO we should keep this stuff. > > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com OK, thanks. Regards, Zhang Mingli
SELECT INTO without columns or star
Hi, hackers When I exec a sql SELECT INTO without columns or * by mistake, it succeeds: select * from t1; a | b ---+--- 1 | 2 2 | 3 3 | 4 (3 rows) select into t2 from t1; SELECT 3 \pset null '(null)' Null display is "(null)". select * from t2; -- (3 rows) It seems that t2 has empty rows but not null. Is it an expected behavior? And what’s the semantic of SELECT INTO without any columns? I also see lots of that SELECT INTO in out test cases like: -- SELECT INTO doesn't support USING SELECT INTO tableam_tblselectinto_heap2 USING heap2 FROM tableam_tbl_heap2; Regards, Zhang Mingli
Re: Fix the miss consideration of tuple_fraction during add_paths_to_append_rel
HI, On Apr 10, 2023, 16:35 +0800, Andy Fan , wrote: > When I am working on "Pushing limit into subqueries of a union" [1], I > found we already have a great infrastructure to support this. For a query > like > > subquery-1 UNION ALL subquery-2 LIMIT 3; > > We have considered the root->tuple_fraction when planning the subqueries > without an extra Limit node as an overhead. But the reason it doesn't work > in my real case is flatten_simple_union_all flat the union all subqueries > into append relation and we didn't handle the root->tuple_fraction during > add_paths_to_append_rel. > > Given the below query for example: > explain analyze > (select * from tenk1 order by hundred) > union all > (select * from tenk2 order by hundred) > limit 3; > > Without the patch: Execution Time: 7.856 ms > with the patch: Execution Time: 0.224 ms > > Any suggestion is welcome. > > [1] https://www.postgresql.org/message-id/11228.1118365833%40sss.pgh.pa.us > > -- > Best Regards > Andy Fan There is spare indent at else if. - if (childrel->pathlist != NIL && + if (cheapest_startup_path && cheapest_startup_path->param_info == NULL) + accumulate_append_subpath(cheapest_startup_path, + &subpaths, NULL); + else if (childrel->pathlist != NIL && childrel->cheapest_total_path->param_info == NULL) accumulate_append_subpath(childrel->cheapest_total_path, &subpaths, NULL); Could we also consider tuple_fraction in partial_pathlist for parallel append? Regards, Zhang Mingli
Re: list of acknowledgments for PG16
Hi, > On Aug 22, 2023, at 17:33, Peter Eisentraut wrote: > > The list of acknowledgments for the PG16 release notes has been committed. > It should show up here sometime: > <https://www.postgresql.org/docs/16/release-16.html#RELEASE-16-ACKNOWLEDGEMENTS>. > As usual, please check for problems such as wrong sorting, duplicate names > in different variants, or names in the wrong order etc. (Our convention is > given name followed by surname.) > > Could you help me with Mingli Zhang -> Zhang Mingli Thanks. Zhang Mingli HashData https://www.hashdata.xyz
Should Explain show Parallel Hash node’s total rows?
Hi, all
Shall we show Parallel Hash node’s total rows of a Parallel-aware HashJoin?
Ex: a non-parallel plan, table simple has 2 rows.
zml=# explain select count(*) from simple r join simple s using (id);
QUERY PLAN
Aggregate (cost=1309.00..1309.01 rows=1 width=8)
-> Hash Join (cost=617.00..1259.00 rows=2 width=0)
Hash Cond: (r.id = s.id )
-> Seq Scan on simple r (cost=0.00..367.00 rows=2 width=4)
-> Hash (cost=367.00..367.00 rows=2 width=4)
-> Seq Scan on simple s (cost=0.00..367.00 rows=2 width=4)
(6 rows)
While a parallel-aware plan:
zml=# explain select count(*) from simple r join simple s using (id);
QUERY PLAN
Finalize Aggregate (cost=691.85..691.86 rows=1 width=8)
-> Gather (cost=691.63..691.84 rows=2 width=8)
Workers Planned: 2
-> Partial Aggregate (cost=691.63..691.64 rows=1 width=8)
-> Parallel Hash Join (cost=354.50..670.80 rows=8333 width=0)
Hash Cond: (r.id = s.id )
-> Parallel Seq Scan on simple r (cost=0.00..250.33
rows=8333 width=4)
-> Parallel Hash (cost=250.33..250.33 rows=8333 width=4)
-> Parallel Seq Scan on simple s
(cost=0.00..250.33 rows=8333 width=4)
(9 rows)
When initial_cost_hashjoin(), we undo the parallel division when parallel ware.
It’s reasonable because a shared hash table should have all the data.
And we also take parallel into account for hash plan’s total rows if it’s
parallel aware.
```
if (best_path->jpath.path.parallel_aware)
{
hash_plan->plan.parallel_aware = true;
hash_plan->rows_total = best_path->inner_rows_total;
}
```
But the Parallel Hash node of plan shows the same rows with subplan, I’m
wandering if it’s more reasonable to show rows_total instead of plan_rows for
Parallel Hash nodes?
For this example,
-> Parallel Hash (rows=2)
-> Parallel Seq Scan on simple s (rows=8333)
Zhang Mingli
HashData https://www.hashdata.xyz
Re: COPY TO (FREEZE)?
HI,
Zhang Mingli
www.hashdata.xyz
On Oct 30, 2023 at 10:58 +0800, Bruce Momjian , wrote:
> On Mon, Oct 30, 2023 at 05:07:48AM +0800, Mingli Zhang wrote:
> >
> > Bruce Momjian 于2023年10月30日周一03:35写道：
> >
> > On Sun, Oct 29, 2023 at 02:50:37PM +0800, Mingli Zhang wrote:
> > > I guess you want to write “cannot be used with COPY TO”
> >
> > You are 100% correct. Updated patch attached.
> >
> > --
> > Bruce Momjian https://momjian.us
> > EDB https://enterprisedb.com
> >
> > Only you can decide what is important to you.
> >
> >
> >
> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> >
> > + errmsg("COPY FREEZE mode cannot be used with COPY FROM")));
> >
> > +
> >
> >
> > COPY FROM-> COPY TO
>
> Agreed, patch attached.
>
> --
> Bruce Momjian https://momjian.us
> EDB https://enterprisedb.com
>
> Only you can decide what is important to you.
LGTM.
Re: COPY TO (FREEZE)?
HI, Zhang Mingli www.hashdata.xyz On Oct 31, 2023 at 03:55 +0800, Bruce Momjian , wrote: > > Sure, updated patch attached. LGTM.
useless LIMIT_OPTION_DEFAULT
Hi, all By reading the codes, I found that we process limit option as LIMIT_OPTION_WITH_TIES when using WITH TIES and all others are LIMIT_OPTION_COUNT by commit 357889eb17bb9c9336c4f324ceb1651da616fe57. And check actual limit node in limit_needed(). There is no need to maintain a useless default limit enum. I remove it and have an install check to verify. Are there any considerations behind this? Shall we remove it for clear as it’s not actually the default option. Zhang Mingli www.hashdata.xyz v1-0001-Remove-useless-LIMIT_OPTION_DEFAULT.patch Description: Binary data
Re: Simplify newNode()
Hi, LGTM. + Assert(size >= sizeof(Node)); /* need the tag, at least */ + result = (Node *) palloc0fast(size); + result->type = tag; + return result; +} How about moving the comments /* need the tag, at least */ after result->type = tag; by the way? Zhang Mingli www.hashdata.xyz
Re: introduce dynamic shared memory registry
Hi, all I see most xxxShmemInit functions have the logic to handle IsUnderPostmaster env. Do we need to consider it in DSMRegistryShmemInit() too? For example, add some assertions. Others LGTM. Zhang Mingli www.hashdata.xyz On Dec 5, 2023 at 11:47 +0800, Nathan Bossart , wrote: > Every once in a while, I find myself wanting to use shared memory in a > loadable module without requiring it to be loaded at server start via > shared_preload_libraries. The DSM API offers a nice way to create and > manage dynamic shared memory segments, so creating a segment after server > start is easy enough. However, AFAICT there's no easy way to teach other > backends about the segment without storing the handles in shared memory, > which puts us right back at square one. > > The attached 0001 introduces a "DSM registry" to solve this problem. The > API provides an easy way to allocate/initialize a segment or to attach to > an existing one. The registry itself is just a dshash table that stores > the handles keyed by a module-specified string. 0002 adds a test for the > registry that demonstrates basic usage. > > I don't presently have any concrete plans to use this for anything, but I > thought it might be useful for extensions for caching, etc. and wanted to > see whether there was any interest in the feature. > > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com
Remove useless GROUP BY columns considering unique index
Hi,
This idea first came from remove_useless_groupby_columns does not need to
record constraint dependencie[0] which points out that
unique index whose columns all have NOT NULL constraints could also take the
work with primary key when removing useless GROUP BY columns.
I study it and implement the idea.
Ex:
create temp table t2 (a int, b int, c int not null, primary key (a, b),
unique(c));
explain (costs off) select * from t2 group by a,b,c;
QUERY PLAN
--
HashAggregate
Group Key: c
-> Seq Scan on t2
The plan drop column a, b as I did a little more.
For the query, as t2 has primary key (a, b), before this patch, we could drop
column c because {a, b} are PK.
And we have an unique index(c) with NOT NULL constraint now, we could drop
column {a, b}, just keep {c}.
While we have multiple choices, group by a, b (c is removed by PK) and group
by c (a, b are removed by unique not null index)
And I implement it to choose the one with less columns so that we can drop as
more columns as possible.
I think it’s good for planner to save some cost like Sort less columns.
There may be better one for some reason like: try to keep PK for planner?
I’m not sure about that and it seems not worth much complex.
The NOT NULL constraint may also be computed from primary keys, ex:
create temp table t2 (a int, b int, c int not null, primary key (a, b),
unique(a));
Primary key(a, b) ensure a is NOT NULL and we have a unique index(a), but it
will introduce more complex to check if a unique index could be used.
I also doubt it worths doing that..
So my patch make it easy: check unique index’s columns, it’s a valid candidate
if all of that have NOT NULL constraint.
And we choose a best one who has the least column numbers in
get_min_unique_not_null_attnos(), as the reason: less columns mean that more
group by columns could be removed.
create temp table t3 (a int, b int, c int not null, d int not null, primary key
(a, b), unique(c, d));
-- Test primary key beats unique not null index.
explain (costs off) select * from t3 group by a,b,c,d;
QUERY PLAN
--
HashAggregate
Group Key: a, b
-> Seq Scan on t3
(3 rows)
create temp table t4 (a int, b int not null, c int not null, d int not null,
primary key (a, b), unique(b, c), unique(d));
-- Test unique not null index with less columns wins.
explain (costs off) select * from t4 group by a,b,c,d;
QUERY PLAN
--
HashAggregate
Group Key: d
-> Seq Scan on t4
(3 rows)
The unique Indices could have overlaps with primary keys and indices themselves.
create temp table t5 (a int not null, b int not null, c int not null, d int not
null, unique (a, b), unique(b, c), unique(a, c, d));
-- Test unique not null indices have overlap.
explain (costs off) select * from t5 group by a,b,c,d;
QUERY PLAN
--
HashAggregate
Group Key: a, b
-> Seq Scan on t5
(3 rows)
[0]https://www.postgresql.org/message-id/flat/CAApHDvrdYa%3DVhOoMe4ZZjZ-G4ALnD-xuAeUNCRTL%2BPYMVN8OnQ%40mail.gmail.com
Zhang Mingli
www.hashdata.xyz
v1-0001-Remove-useless-GROUP-BY-columns-considering-unique-i.patch
Description: Binary data
Re: Why do parallel scans require syncscans (but not really)?
Hi,
Zhang Mingli
www.hashdata.xyz
Hi, Tomas
On Dec 31, 2023 at 07:10 +0800, Tomas Vondra ,
wrote:
> Sadly, there's no explanation why parallel scans do not allow disabling
> sync scans just like serial scans - and it's not quite obvious to me.
Feel confused too.
```
Assert(!IsBootstrapProcessingMode());
Assert(allow_sync);
snapshot = scan->rs_snapshot;
```
I dig this for a while and found that there a close relationship between field
phs_syncscan(For parallel scan) and the allow_sync flag.
In table_block_parallelscan_initialize() ParallelTableScanDescData.phs_syncscan
is set.
/* compare phs_syncscan initialization to similar logic in initscan */
bpscan->base.phs_syncscan = synchronize_seqscans &&
!RelationUsesLocalBuffers(rel) &&
bpscan->phs_nblocks > NBuffers / 4;
And the allow_sync is always set to true in initscan(), when phs_syncscan is
not NULL.
if (scan->rs_base.rs_parallel != NULL)
{
/* For parallel scan, believe whatever ParallelTableScanDesc
says. */
if (scan->rs_base.rs_parallel->phs_syncscan)
scan->rs_base.rs_flags |= SO_ALLOW_SYNC;
else
scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC;
}
And phs_syncscan is used in
table_block_parallelscan_startblock_init(),table_block_parallelscan_nextpage()
to do sth of syncscan.
Back to the Assertion, else branch means param scan(parallel scan desc) is not
null and we are in parallel scan mode.
else
{
/*
* Parallel index build.
*
* Parallel case never registers/unregisters own snapshot.
Snapshot
* is taken from parallel heap scan, and is SnapshotAny or an
MVCC
* snapshot, based on same criteria as serial case.
*/
Assert(!IsBootstrapProcessingMode());
Assert(allow_sync);
snapshot = scan->rs_snapshot;
}
Agree with you that: why all parallel plans should / must be synchronized?
Parallel scan should have a choice about syncscan.
Besides that I think there is a risk Assert(allow_sync), at least should use
phs_syncscan field here to judge if allow_sync is true according to above.
So I guess, there should be an Assertion failure of Assert(allow_sync) in
theory when we use a parallel scan but phs_syncscan is false.
/* compare phs_syncscan initialization to similar logic in initscan */
bpscan->base.phs_syncscan = synchronize_seqscans &&
!RelationUsesLocalBuffers(rel) &&
bpscan->phs_nblocks > NBuffers / 4;
However, I didn’t produce it because phs_syncscan is set according to data
size, even with some parallel cost GUCs set to 0.
And if there is not enough data, we usually do not choose a parallel plan,
like this case: build a index with parallel scan on underlying tables.
Re: Minor cleanup for search path cache
Hi, Zhang Mingli www.hashdata.xyz On Jan 2, 2024 at 05:38 +0800, Tom Lane , wrote: > I happened to notice that there is a not-quite-theoretical crash > hazard in spcache_init(). If we see that SPCACHE_RESET_THRESHOLD > is exceeded and decide to reset the cache, but then nsphash_create > fails for some reason (perhaps OOM), an error will be thrown > leaving the SearchPathCache pointer pointing at already-freed > memory. Next time through, we'll try to dereference that dangling > pointer, potentially causing SIGSEGV, or worse we might find a > value less than SPCACHE_RESET_THRESHOLD and decide that the cache > is okay despite having been freed. > > The fix of course is to make sure we reset the pointer variables > *before* the MemoryContextReset. > > I also observed that the code seems to have been run through > pgindent without fixing typedefs.list, making various places > uglier than they should be. > > The attached proposed cleanup patch fixes those things and in > passing improves (IMO anyway) some comments. I assume it wasn't > intentional to leave two copies of the same comment block in > check_search_path(). > > regards, tom lane > Only me? zml@localhashdata postgres % git apply minor-search-path-cache-cleanup.patch error: patch failed: src/backend/catalog/namespace.c:156 error: src/backend/catalog/namespace.c: patch does not apply error: patch failed: src/tools/pgindent/typedefs.list:2479 error: src/tools/pgindent/typedefs.list: patch does not apply I’m in commit 9a17be1e24 Allow upgrades to preserve the full subscription's state
Re: weird GROUPING SETS and ORDER BY behaviour
Hi, Zhang Mingli www.hashdata.xyz On Jan 6, 2024 at 01:38 +0800, Geoff Winkless , wrote: > > Am I missing some reason why the first set isn't sorted as I'd hoped? Woo, it’s a complex order by, I try to understand your example. And I think the order is right, what’s your expected order result? ``` ORDER BY CASE WHEN GROUPING(test1.n)=0 THEN test1.n ELSE NULL END NULLS FIRST, CASE WHEN GROUPING(concat(test1.n, (SELECT x FROM test2 WHERE seq=test1.seq)))=0 THEN concat(test1.n, (SELECT x FROM test2 WHERE seq=test1.seq)) ELSE NULL END NULLS FIRST; ``` You want to Order by a, b where a is: CASE WHEN GROUPING(test1.n)=0 THEN test1.n ELSE NULL END NULLS FIRST. GROUPING(test1.n)=0 means that your are within grouping set test1.n and the value is test1.n, so results of another grouping set b is NULL, and you specific NULL FIRST. So your will first get the results of grouping set b while of course, column gp_n GROUPING(test1.n) is 1. The result is very right. gp_n | gp_conc | n | concat --+-+--+ 1 | 0 | NULL | n5x1 1 | 0 | NULL | n4x2 1 | 0 | NULL | n3x3 1 | 0 | NULL | n2x4 1 | 0 | NULL | n1x5 0 | 1 | n1 | NULL 0 | 1 | n2 | NULL 0 | 1 | n3 | NULL 0 | 1 | n4 | NULL 0 | 1 | n5 | NULL (10 rows) NB: the Grouping bit is set to 1 when this column is not included. https://www.postgresql.org/docs/current/functions-aggregate.html GROUPING ( group_by_expression(s) ) → integer Returns a bit mask indicating which GROUP BY expressions are not included in the current grouping set. Bits are assigned with the rightmost argument corresponding to the least-significant bit; each bit is 0 if the corresponding expression is included in the grouping criteria of the grouping set generating the current result row, and 1 if it is not included I guess you misunderstand it? And your GROUPING target entry seems misleading, I modify it to: SELECT GROUPING(test1.n, (concat(test1.n, (SELECT x FROM test2 WHERE seq=test1.seq::bit(2), test1.n, CONCAT(test1.n, (SELECT x FROM test2 WHERE seq=test1.seq)) FROM test1 …skip To show the grouping condition: grouping | n | concat --+--+ 10 | NULL | n5x1 10 | NULL | n4x2 10 | NULL | n3x3 10 | NULL | n2x4 10 | NULL | n1x5 01 | n1 | NULL 01 | n2 | NULL 01 | n3 | NULL 01 | n4 | NULL 01 | n5 | NULL (10 rows)
Re: weird GROUPING SETS and ORDER BY behaviour
Hi, Zhang Mingli www.hashdata.xyz On Jan 6, 2024 at 23:38 +0800, Geoff Winkless , wrote: > > I was hoping to see > > gp_n | gp_conc | n | concat > --+-+--+ > 1 | 0 | NULL | n1x5 > 1 | 0 | NULL | n2x4 > 1 | 0 | NULL | n3x3 > 1 | 0 | NULL | n4x2 > 1 | 0 | NULL | n5x1 > 0 | 1 | n1 | NULL > 0 | 1 | n2 | NULL > 0 | 1 | n3 | NULL > 0 | 1 | n4 | NULL > 0 | 1 | n5 | NULL > > because when gp_conc is 0, it should be ordering by the concat() value. Hi, I misunderstand and thought you want to see the rows of gp_n = 0 first. So you’re not satisfied with the second key of Order By. I simply the SQL to show that the difference exists: SELECT GROUPING(test1.n) AS gp_n, GROUPING(concat(test1.n, (SELECT x FROM test2 WHERE seq=test1.seq))) AS gp_conc, test1.n, CONCAT(test1.n, (SELECT x FROM test2 WHERE seq=test1.seq)) FROM test1 GROUP BY GROUPING SETS( (test1.n), (concat(test1.n, (SELECT x FROM test2 WHERE seq=test1.seq))) ) HAVING n is NULL ORDER BY concat(test1.n, (SELECT x FROM test2 WHERE seq=test1.seq)) NULLS FIRST; gp_n | gp_conc | n | concat --+-+--+ 1 | 0 | NULL | n1x5 1 | 0 | NULL | n2x4 1 | 0 | NULL | n3x3 1 | 0 | NULL | n4x2 1 | 0 | NULL | n5x1 (5 rows) This is what you want, right? And if there is a CASE WHEN, the order changed: SELECT GROUPING(test1.n) AS gp_n, GROUPING(concat(test1.n, (SELECT x FROM test2 WHERE seq=test1.seq))) AS gp_conc, test1.n, CONCAT(test1.n, (SELECT x FROM test2 WHERE seq=test1.seq)) FROM test1 GROUP BY GROUPING SETS( (test1.n), (concat(test1.n, (SELECT x FROM test2 WHERE seq=test1.seq))) ) HAVING n is NULL ORDER BY CASE WHEN true THEN concat(test1.n, (SELECT x FROM test2 WHERE seq=test1.seq)) END NULLS FIRST; gp_n | gp_conc | n | concat --+-+--+ 1 | 0 | NULL | n5x1 1 | 0 | NULL | n4x2 1 | 0 | NULL | n3x3 1 | 0 | NULL | n2x4 1 | 0 | NULL | n1x5 (5 rows) I haven’t dinged into this and it seems sth related with CASE WHEN. A case when true will change the order.
Skip Orderby Execution for Materialized Views
Hi, all When create or refresh a Materialized View, if the view’s query has order by, we may sort and insert the sorted data into view. Create Materialized View mv1 as select c1, c2 from t1 order by c2; Refresh Materialized View mv1; And it appears that we could get ordered data when select from Materialized View; Select * from mv1; But it’s not true if we use other access methods, or we choose a parallel seqscan plan. A non-parallel seqscan on heap table appears ordered as we always create new rel file and swap them, in my opinion, it’s more like a free lunch. So, conclusion1: We couldn’t rely on the `ordered-data` even the mv’s sql has order by clause, is it right? And if it’s true, shall we skip the order by clause for Materialized View when executing create/refresh statement? Materialized View’s order by clause could be skipped if 1. Order by clause is on the top query level 2. There is no real limit clause The benefit is the query may be speeded up without sort nodes each time creating/refreshing Materialized View. A simple results: create table t1 as select i as c1 , i/2 as c2 , i/5 as c3 from generate_series(1, 10) i; create materialized view mvt1_order as select c1, c2, c3 from t1 order by c2, c3, c1 asc Without this patch: zml=# refresh materialized view mvt1_order; REFRESH MATERIALIZED VIEW Time: 228.548 ms zml=# refresh materialized view mvt1_order; REFRESH MATERIALIZED VIEW Time: 230.374 ms zml=# refresh materialized view mvt1_order; REFRESH MATERIALIZED VIEW Time: 217.079 ms With this patch: zml=# refresh materialized view mvt1_order; REFRESH MATERIALIZED VIEW Time: 192.409 ms zml=# refresh materialized view mvt1_order; REFRESH MATERIALIZED VIEW Time: 204.398 ms zml=# refresh materialized view mvt1_order; REFRESH MATERIALIZED VIEW Time: 197.510 ms Zhang Mingli www.hashdata.xyz v1-01-Skip-Orderby-clause-execution-for-Materialized-Views.patch Description: Binary data
Re: Skip Orderby Execution for Materialized Views
HI, > On Oct 1, 2023, at 22:54, Tom Lane wrote: > > For one example, > you can't just remove the sort clause if the query uses DISTINCT ON Hi, Tom, got it, thanks, Zhang Mingli HashData https://www.hashdata.xyz
Re: Free list same_input_transnos in preprocess_aggref
HI, On Nov 7, 2022, 04:12 +0800, Tom Lane , wrote: > > The NIL lists are of course occupying no storage. The two one-element > lists are absolutely, completely negligible in the context of planning > any nontrivial statement. Even the aggtransinfos list that is the > primary output of preprocess_aggref will dwarf that; and we leak > similarly small data structures in probably many hundred places in > the planner. > > I went a bit further and ran the core regression tests, then aggregated > the results: > > $ grep 'leaking list' postmaster.log | sed 's/.*] //' | sort | uniq -c > 4516 LOG: leaking list of length 0 > 95 LOG: leaking list of length 1 > 15 LOG: leaking list of length 2 > > You can quibble of course about how representative the regression tests > are, but there's sure no evidence at all here that we'd be saving > anything measurable. > > If anything, I'd be inclined to get rid of the > > list_free(*same_input_transnos); > > in find_compatible_agg, because it seems like a waste of code on > the same grounds. Instrumenting that in the same way, I find > that it's not reached at all in your example, while the > regression tests give > > 49 LOG: freeing list of length 0 > 2 LOG: freeing list of length 1 > Thanks for the investigation. Yeah, this patch is negligible. I’ll withdraw it in CF. Regards, Zhang Mingli
Re: [PoC] Reducing planning time when tables have many partitions
HI, Regards, Zhang Mingli On Nov 7, 2022, 14:26 +0800, Tom Lane , wrote: > Andrey Lepikhov writes: > > I'm still in review of your patch now. At most it seems ok, but are you > > really need both eq_sources and eq_derives lists now? > > Didn't we just have this conversation? eq_sources needs to be kept > separate to support the "broken EC" logic. We don't want to be > regurgitating derived clauses as well as originals in that path. > Aha, we have that conversation in another thread(Reducing duplicativeness of EquivalenceClass-derived clauses ) : https://www.postgresql.org/message-id/644164.1666877342%40sss.pgh.pa.us
Re: [patch] Adding an assertion to report too long hash table name
Hi,
On Sep 29, 2022, 09:38 +0800, Xiaoran Wang , wrote:
> Hi,
> The max size for the shmem hash table name is SHMEM_INDEX_KEYSIZE - 1. but
> when the caller uses a longer hash table name, it doesn't report any error,
> insteadit just uses the first SHMEM_INDEX_KEYSIZE -1 chars as the hash table
> name.
> I created some shmem hash tables with the same prefix which was longer
> thanSHMEM_INDEX_KEYSIZE - 1, and the size of those hash tables were the
> same,then only one hash table was created. But I thought those hash tables
> were createdsuccessfully.
> I know this is a corner case, but it's difficult to figure it out when run
> into it. So I addan assertion to prevent it.
>
> Thanks,Xiaoran
Seems Postgres doesn’t have a case that strlen(name) >= SHMEM_INDEX_KEYSIZE(48).
The max length of name I found is 29:
```
ShmemInitHash("Shared Buffer Lookup Table”
```
But it will help for other Databases built on Postgres or later Postgres in
case of forgetting to update SHMEM_INDEX_KEYSIZE
when new shmem added with a name longer than current SHMEM_INDEX_KEYSIZE.
And we don’t have such assertion now.
So, +1 for the patch.
Regards,
Zhang Mingli
What’s the usage of SO_TYPE_ANALYZE
Hi, hackers
I found that we set the SO_TYPE_ANALYZE option in table_beginscan_analyze()
static inline TableScanDesc
table_beginscan_analyze(Relation rel)
{
uint32 flags = SO_TYPE_ANALYZE;
return rel->rd_tableam->scan_begin(rel, NULL, 0, NULL, NULL, flags);
}
But I didn’t find a place to handle that option.
Do we miss something? ex: forget handle it in table_endscan.
Else, it’s not used at all.
The first commit introduced this option is c3b23ae457.
Other commits modify this option just changed the enum order.
Regards,
Zhang Mingli
Remove unused param rte in set_plain_rel_pathlist
Hi, hackers Param RangeTblEntry *rte in function set_plain_rel_pathlist is not used at all. I look at the commit e2fa76d80b(10 years ago), it’s useless since then. Add a path to remove it. Regards, Zhang Mingli v1-0001-Remove-unused-param-rte-in-set_plain_rel_pathlist.patch Description: Binary data
Add more docs for pg_surgery?
Hi, hackers
heap_force_kill/heap_force_freeze doesn’t consider other transactions that are
using the same tuples even with tuple-locks.
The functions may break transaction semantic, ex:
session1
```
create table htab(id int);
insert into htab values (100), (200), (300), (400), (500);
```
session2
```
begin isolation level repeatable read;
select * from htab for share;
id
-
100
200
300
400
500
(5 rows)
```
session1
```
select heap_force_kill('htab'::regclass, ARRAY['(0, 1)']::tid[]);
heap_force_kill
-
(1 row)
```
session2
```
select * from htab for share;
id
-
200
300
400
500
(4 rows)
```
session2 should get the same results as it's repeatable read isolation level.
By reading the doc:
```
The pg_surgery module provides various functions to perform surgery on a
damaged relation. These functions are unsafe by design and using them may
corrupt (or further corrupt) your database. For example, these functions can
easily be used to make a table inconsistent with its own indexes, to cause
UNIQUE or FOREIGN KEY constraint violations, or even to make tuples visible
which, when read, will cause a database server crash. They should be used with
great caution and only as a last resort.
```
I know they are powerful tools, but also a little surprise with the above
example.
Should we add more docs to tell the users that the tool will change the tuples
anyway even there are tuple-locks on them?
Regards,
Zhang Mingli
Re: Add more docs for pg_surgery?
Regards, Zhang Mingli On Sep 27, 2022, 00:47 +0800, Ashutosh Sharma , wrote: > > And further it looks like you are doing an > experiment on undamaged relation which is not recommended as > documented. Yeah. > If the relation would have been damaged, you probably may > not be able to access it. > That make some sense. > -- > With Regards, > Ashutosh Sharma.
Re: Summary function for pg_buffercache
Regards,
Zhang Mingli
On Sep 28, 2022, 21:50 +0800, Melih Mutlu , wrote:
> Hi all,
>
> The patch needed a rebase due to recent changes on pg_buffercache.
> You can find the updated version attached.
>
> Best,
> Melih
>
>
```
+
+ if (buffers_used != 0)
+ usagecount_avg = usagecount_avg / buffers_used;
+
+ memset(nulls, 0, sizeof(nulls));
+ values[0] = Int32GetDatum(buffers_used);
+ values[1] = Int32GetDatum(buffers_unused);
+ values[2] = Int32GetDatum(buffers_dirty);
+ values[3] = Int32GetDatum(buffers_pinned);
+
+ if (buffers_used != 0)
+ {
+ usagecount_avg = usagecount_avg / buffers_used;
+ values[4] = Float4GetDatum(usagecount_avg);
+ }
+ else
+ {
+ nulls[4] = true;
+ }
```
Why compute usagecount_avg twice?
Re: Summary function for pg_buffercache
Hi, On Sep 28, 2022, 22:41 +0800, Melih Mutlu , wrote: > > > Zhang Mingli , 28 Eyl 2022 Çar, 17:31 tarihinde şunu > yazdı: > > Why compute usagecount_avg twice? > > I should have removed the first one, but I think I missed it. > Nice catch. > > Attached an updated version. > > Thanks, > Melih > Hmm, I just apply v13 patch but failed. Part of errors: ``` error: patch failed: contrib/pg_buffercache/pg_buffercache.control:1 error: contrib/pg_buffercache/pg_buffercache.control: patch does not apply Checking patch contrib/pg_buffercache/pg_buffercache_pages.c... error: while searching for: */ PG_FUNCTION_INFO_V1(pg_buffercache_pages); PG_FUNCTION_INFO_V1(pg_buffercache_pages_v1_4); ``` Rebase on master and then apply our changes again? Regards, Zhang Mingli >
Re: Refactor UnpinBuffer()
HI, On Sep 29, 2022, 05:08 +0800, Nathan Bossart , wrote: > On Wed, Sep 28, 2022 at 08:14:23PM +0300, Aleksander Alekseev wrote: > > + ResourceOwnerForgetBuffer(CurrentResourceOwner, b); > > + > > /* not moving as we're likely deleting it soon anyway */ > > ref = GetPrivateRefCountEntry(b, false); > > Assert(ref != NULL); > > - > > - if (fixOwner) > > - ResourceOwnerForgetBuffer(CurrentResourceOwner, b); +1, Good catch. > > Is it safe to move the call to ResourceOwnerForgetBuffer() to before the > call to GetPrivateRefCountEntry()? From my quick skim of the code, it > seems like it should be safe, but I thought I'd ask the question. Same question, have a look, it doesn’t seem to matter. Regards, Zhang Mingli
Re: Summary function for pg_buffercache
Hi, On Sep 28, 2022, 23:20 +0800, Melih Mutlu , wrote: > Hi, > > Seems like the commit a448e49bcbe40fb72e1ed85af910dd216d45bad8 reverts the > changes on pg_buffercache. > > > Why compute usagecount_avg twice? > Then, I'm going back to v11 + the fix for this. > > Thanks, > Melih Looks good to me. Regards, Zhang Mingli
Re: Issue in GIN fast-insert: XLogBeginInsert + Read/LockBuffer ordering
HI, On Sep 8, 2022, 19:08 +0800, Matthias van de Meent , wrote: > In general, this works fine, except that in ginHeapTupleFastInsert we > call XLogBeginInsert() before the last of the buffers for the eventual > record was read, thus creating a path where eviction is possible in a > `begininsert_called = true` context. That breaks our current code by > being unable to evict (WAL-log) the dirtied hint pages. Does it break Postgres or Neon? I look around the codes and as far as I can see, dirty pages could be flushed whether begininsert_called is true or false. > > PFA a patch that rectifies this issue, by moving the XLogBeginInsert() > down to where 1.) we have all relevant buffers pinned and locked, and > 2.) we're in a critical section, making that part of the code > consistent with the general scheme for XLog insertion. +1, Make sense. Regards, Zhang Mingli
Re: Issue in GIN fast-insert: XLogBeginInsert + Read/LockBuffer ordering
Hi, On Sep 8, 2022, 19:08 +0800, Matthias van de Meent , wrote: > > PFA a patch that rectifies this issue, by moving the XLogBeginInsert() > down to where 1.) we have all relevant buffers pinned and locked, and > 2.) we're in a critical section, making that part of the code > consistent with the general scheme for XLog insertion. > In the same function, there is disorder of XLogBeginInsert and START_CRIT_SECTION. ``` collectordata = ptr = (char *) palloc(collector->sumsize); data.ntuples = collector->ntuples; if (needWal) XLogBeginInsert(); START_CRIT_SECTION(); ``` Shall we adjust that too? Regards, Zhang Mingli
Re: get rid of Abs()
Hi, On Oct 4, 2022, 15:07 +0800, Peter Eisentraut , wrote: > I was wondering why we have a definition of Abs() in c.h when there are > more standard functions such as abs() and fabs() in widespread use. I > think this one is left over from pre-ANSI-C days. The attached patches > replace all uses of Abs() with more standard functions. > > The first patch installs uses of abs() and fabs(). These are already in > use in the tree and should be straightforward. > > The next two patches install uses of llabs() and fabsf(), which are not > in use yet. But they are in C99. > > The last patch removes the definition of Abs(). > > > Fun fact: The current definition > > #define Abs(x) ((x) >= 0 ? (x) : -(x)) > > is slightly wrong for floating-point values. Abs(-0.0) returns -0.0, > but fabs(-0.0) returns +0.0. +1, Like patch3, also found some places where could use fabsf instead of fabs if possible, add a patch to replace them. Regards, Zhang Mingli v2-0005-replace-fabs-with-fabsf-if-possible.patch Description: Binary data
Re: Issue in GIN fast-insert: XLogBeginInsert + Read/LockBuffer ordering
HI, On Oct 12, 2022, 10:09 +0800, Michael Paquier , wrote: > > Nice catches, both of you. Let's adjust everything spotted in one > shot. Matthias' patch makes the ordering right, but the > initialization path a bit harder to follow when using a separate list. > The code is short so it does not strike me as a big deal, and it comes > from the fact that we need to lock an existing buffer when merging two > lists. For the branch where we insert into a tail page, the pages are > already locked but it looks to be enough to move XLogBeginInsert() > before the first XLogRegisterBuffer() call. > > Would any of you like to write a patch? > -- > Michael Patch added. Regards, Zhang Mingli v2-0001-Fix-GIN-fast-path-XLogBeginInsert-and-Read-LockBu.patch Description: Binary data
Documentation refinement for Parallel Scans
Hi, Found documents about parallel scan may be not so accurate. As said in parallel.smgl: ``` In a parallel sequential scan, the table's blocks will be divided among the cooperating processes. Blocks are handed out one at a time, so that access to the table remains sequential. ``` To my understanding, this was right before. Because we return one block if a worker ask for before commit 56788d2156. As comments inside table_block_parallelscan_nextpage: ``` Earlier versions of this would allocate the next highest block number to the next worker to call this function. ``` And from commit 56788d2156, each parallel worker will try to get ranges of blocks “chunks". Access to the table remains sequential inside each worker’s process, but not across all workers or the parallel query. Shall we update the documents? Regards, Zhang Mingli
Re: Add 64-bit XIDs into PostgreSQL 15
Hi, On Oct 22, 2022, 00:09 +0800, Maxim Orlov , wrote: > > > > Done! Thanks! Here is the rebased version. > > > > This version has bug fix for multixact replication. Previous versions of > > the patch set does not write pd_multi_base in WAL. Thus, this field was set > > to 0 upon WAL reply on replica. > > This caused replica to panic. Fix this by adding pd_multi_base of a page > > into WAL. Appropriate tap test is added. > > > > Also, add refactoring and improvements in heapam.c in order to reduce diff > > and make it more "tidy". > > > > Reviews and opinions are very welcome! > > > > -- > > Best regards, > > Maxim Orlov. Found some outdate code comments around several variables, such as xidWrapLimit/xidWarnLimit/xidStopLimt. These variables are not used any more. I attach an additional V48-0009 patch as they are just comments, apply it if you want to. Regards, Zhang Mingli v48-0009-remove-some-outdate-codes-comments-about-xidWrap.patch Description: Binary data
doubt about FullTransactionIdAdvance()
Hi, hackers
I don't quite understand FullTransactionIdAdvance(), correct me if I’m wrong.
/*
* Advance a FullTransactionId variable, stepping over xids that would appear
* to be special only when viewed as 32bit XIDs.
*/
static inline void
FullTransactionIdAdvance(FullTransactionId *dest)
{
dest->value++;
/* see FullTransactionIdAdvance() */
if (FullTransactionIdPrecedes(*dest, FirstNormalFullTransactionId))
return;
while (XidFromFullTransactionId(*dest) < FirstNormalTransactionId)
dest->value++;
}
#define XidFromFullTransactionId(x) ((x).value)
#define FullTransactionIdPrecedes(a, b) ((a).value < (b).value)
Can the codes reach line: while (XidFromFullTransactionId(*dest) <
FirstNormalTransactionId)?
As we will return if (FullTransactionIdPrecedes(*dest,
FirstNormalFullTransactionId)), and the two judgements seem equal.
Another confusion is the comments: /* see FullTransactionIdAdvance() */, is its
own itself.
Could anyone explain this? Thanks in advance.
Regards,
Zhang Mingli
Re: doubt about FullTransactionIdAdvance()
Hi, Andres
On Oct 24, 2022, 01:16 +0800, Andres Freund , wrote:
> Hi,
>
> On 2022-10-22 11:32:47 +0800, Zhang Mingli wrote:
> > Hi, hackers
> >
> > I don't quite understand FullTransactionIdAdvance(), correct me if I’m
> > wrong.
> >
> >
> > /*
> > * Advance a FullTransactionId variable, stepping over xids that would
> > appear
> > * to be special only when viewed as 32bit XIDs.
> > */
> > static inline void
> > FullTransactionIdAdvance(FullTransactionId *dest)
> > {
> > dest->value++;
> >
> > /* see FullTransactionIdAdvance() */
> > if (FullTransactionIdPrecedes(*dest, FirstNormalFullTransactionId))
> > return;
> >
> > while (XidFromFullTransactionId(*dest) < FirstNormalTransactionId)
> > dest->value++;
> > }
> >
> > #define XidFromFullTransactionId(x) ((x).value)
> > #define FullTransactionIdPrecedes(a, b) ((a).value < (b).value)
> >
> > Can the codes reach line: while (XidFromFullTransactionId(*dest) <
> > FirstNormalTransactionId)?
> > As we will return if (FullTransactionIdPrecedes(*dest,
> > FirstNormalFullTransactionId)), and the two judgements seem equal.
> > Another confusion is the comments: /* see FullTransactionIdAdvance() */, is
> > its own itself.
> > Could anyone explain this? Thanks in advance.
>
> FullTransactionId is 64bit. An "old school" xid is 32bit. The first branch is
> to protect against the special fxids that are actually below
> FirstNormalFullTransactionId:
>
> if (FullTransactionIdPrecedes(*dest, FirstNormalFullTransactionId))
> return;
>
> The second branch is to protect against 64bit xids that would yield a 32bit
> xid below FirstNormalTransactionId after truncating to 32bit:
>
> while (XidFromFullTransactionId(*dest) < FirstNormalTransactionId)
> dest->value++;
>
> E.g. we don't want to modify the 64bit xid 0 (meaning InvalidTransactionId) as
> it has special meaning. But we have to make sure that e.g. the 64bit xid
> 0x1 won't exist, as it'd also result in InvalidTransactionId if
> truncated to 32bit.
>
>
> However, it looks like this comment:
> /* see FullTransactionIdAdvance() */
> if (FullTransactionIdPrecedes(*dest, FirstNormalFullTransactionId))
> return;
>
> is bogus, and it's my fault. Looks like it's intending to reference
> FullTransactionIdRetreat().
>
> Greetings,
>
> Andres Freund
Now I get it, thanks for your explanation.
Regards,
Zhang Mingli
Re: Reducing duplicativeness of EquivalenceClass-derived clauses
HI,
On Oct 26, 2022, 06:09 +0800, Tom Lane , wrote:
> While fooling with my longstanding outer-join variables changes
> (I am making progress on that, honest), I happened to notice that
> equivclass.c is leaving some money on the table by generating
> redundant RestrictInfo clauses. It already attempts to not generate
> the same clause twice, which can save a nontrivial amount of work
> because we cache selectivity estimates and so on per-RestrictInfo.
> I realized though that it will build and return a clause like
> "a.x = b.y" even if it already has "b.y = a.x". This is just
> wasteful. It's always been the case that equivclass.c will
> produce clauses that are ordered according to its own whims.
> Consumers that need the operands in a specific order, such as
> index scans or hash joins, are required to commute the clause
> to be the way they want it while building the finished plan.
> Therefore, it shouldn't matter which order of the operands we
> return, and giving back the commutator clause if available could
> potentially save as much as half of the selectivity-estimation
> work we do with these clauses subsequently.
>
> Hence, PFA a patch that adjusts create_join_clause() to notice
> commuted as well as exact matches among the EquivalenceClass's
> existing clauses. This results in a number of changes visible in
> regression test cases, but they're all clearly inconsequential.
>
> The only thing that I think might be controversial here is that
> I dropped the check for matching operator OID. To preserve that,
> we'd have needed to use get_commutator() in the reverse-match cases,
> which it seemed to me would be a completely unjustified expenditure
> of cycles. The operators we select for freshly-generated clauses
> will certainly always match those of previously-generated clauses.
> Maybe there's a chance that they'd not match those of ec_sources
> clauses (that is, the user-written clauses we started from), but
> if they don't and that actually makes any difference then surely
> we are talking about a buggy opclass definition.
>
> I've not bothered to make any performance tests to see if there's
> actually an easily measurable gain here. Saving some duplicative
> selectivity estimates could be down in the noise ... but it's
> surely worth the tiny number of extra tests added here.
>
> Comments?
>
> regards, tom lane
>
Make sense.
How about combine ec->ec_sources and ec->derives as one list for less codes?
```
foreach(lc, list_union(ec->ec_sources, ec->ec_derives))
{
rinfo = (RestrictInfo *) lfirst(lc);
if (rinfo->left_em == leftem &&
rinfo->right_em == rightem &&
rinfo->parent_ec == parent_ec)
return rinfo;
if (rinfo->left_em == rightem &&
rinfo->right_em == leftem &&
rinfo->parent_ec == parent_ec)
return rinfo;
}
```
I have a try, it will change some in join.out and avoid changes in tidscan.out.
Regards,
Zhang Mingli
>
Re: Reducing duplicativeness of EquivalenceClass-derived clauses
Hi, On Oct 27, 2022, 21:29 +0800, Tom Lane , wrote: > Zhang Mingli writes: > > How about combine ec->ec_sources and ec->derives as one list for less codes? > > Keeping them separate is required for the broken-EC code paths. > Even if it weren't, I wouldn't merge them just to save a couple > of lines of code --- I think it's useful to be able to tell which > clauses the EC started from. > > regards, tom lane Got it, thanks. Regards, Zhang Mingli
Re: Lock on ShmemVariableCache fields?
HI,
On Oct 31, 2022, 10:48 +0800, Japin Li , wrote:
>
> Hi, hackers
>
> The VariableCacheData says nextOid and oidCount are protected by
> OidGenLock. However, we update them without holding the lock on
> OidGenLock in BootStrapXLOG(). Same as nextXid, for other fields
> that are protected by XidGenLock, it holds the lock, see
> SetTransactionIdLimit().
>
> void
> BootStrapXLOG(void)
> {
> [...]
>
> ShmemVariableCache->nextXid = checkPoint.nextXid;
> ShmemVariableCache->nextOid = checkPoint.nextOid;
> ShmemVariableCache->oidCount = 0;
>
> [...]
>
> SetTransactionIdLimit(checkPoint.oldestXid, checkPoint.oldestXidDB);
>
> [...]
> }
>
> I also find a similar code in StartupXLOG(). Why we don't hold the lock
> on OidGenLock when updating ShmemVariableCache->nextOid and
> ShmemVariableCache->oidCount?
>
> If the lock is unnecessary, I think adding some comments is better.
>
> --
> Regrads,
> Japin Li.
> ChengDu WenWu Information Technology Co.,Ltd.
>
>
As its name BootStrapXLOG, it’s used in BootStrap mode to initialize the
template database.
The process doesn’t speak SQL and the database is not ready.
There won’t be concurrent access to variables.
Regards,
Zhang Mingli
>
Fix annotations nextFullXid
Hi,VariableCacheData.nextFullXid is renamed to nextXid in commit https://github.com/postgres/postgres//commit/fea10a64340e529805609126740a540c8f9daab4Fix the annotations for less confusion.Regards,Zhang Mingli Fix-annotations-nextFullXid.patch Description: Binary data
Re: optimize lookups in snapshot [sub]xip arrays
Hi, all
>
> if (!snapshot->suboverflowed)
> {
> /* we have full data, so search subxip */
> - int32 j;
> -
> - for (j = 0; j < snapshot->subxcnt; j++)
> - {
> - if (TransactionIdEquals(xid,
> snapshot->subxip[j]))
> - return true;
> - }
> + if (XidInXip(xid, snapshot->subxip, snapshot->subxcnt,
> + &snapshot->subxiph))
> + return true;
>
> /* not there, fall through to search xip[] */
> }
If snaphost->suboverflowed is false then the subxcnt must be less than
PGPROC_MAX_CACHED_SUBXIDS which is 64 now.
And we won’t use hash if the xcnt is less than XIP_HASH_MIN_ELEMENTS which is
128 currently during discussion.
So that, subxid’s hash table will never be used, right?
Regards,
Zhang Mingli
> On Jul 14, 2022, at 01:09, Nathan Bossart wrote:
>
> Hi hackers,
>
> A few years ago, there was a proposal to create hash tables for long
> [sub]xip arrays in snapshots [0], but the thread seems to have fizzled out.
> I was curious whether this idea still showed measurable benefits, so I
> revamped the patch and ran the same test as before [1]. Here are the
> results for 60₋second runs on an r5d.24xlarge with the data directory on
> the local NVMe storage:
>
> writers HEAD patch diff
>
> 16 659 664+1%
> 32 645 663+3%
> 64 659 692+5%
> 128 641 716+12%
> 256 619 610-1%
> 512 530 702+32%
> 768 469 582+24%
> 1000 367 577+57%
>
> As before, the hash table approach seems to provide a decent benefit at
> higher client counts, so I felt it was worth reviving the idea.
>
> The attached patch has some key differences from the previous proposal.
> For example, the new patch uses simplehash instead of open-coding a new
> hash table. Also, I've bumped up the threshold for creating hash tables to
> 128 based on the results of my testing. The attached patch waits until a
> lookup of [sub]xip before generating the hash table, so we only need to
> allocate enough space for the current elements in the [sub]xip array, and
> we avoid allocating extra memory for workloads that do not need the hash
> tables. I'm slightly worried about increasing the number of memory
> allocations in this code path, but the results above seemed encouraging on
> that front.
>
> Thoughts?
>
> [0] https://postgr.es/m/35960b8af917e9268881cd8df3f88320%40postgrespro.ru
> [1] https://postgr.es/m/057a9a95-19d2-05f0-17e2-f46ff20e9b3e%402ndquadrant.com
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>
Re: ReadRecentBuffer() is broken for local buffer
Nice catch, LGTM.
> On Jul 25, 2022, at 02:22, Heikki Linnakangas wrote:
>
> ReadRecentBuffer() doesn't work for local buffers, i.e. for temp tables. The
> bug is pretty clear if you look at the code:
>
> if (BufferIsLocal(recent_buffer))
> {
> - bufHdr = GetBufferDescriptor(-recent_buffer - 1);
> + bufHdr = GetLocalBufferDescriptor(-recent_buffer - 1);
>
> The code after that looks suspicious, too. It increases the usage count even
> if the buffer was already pinned. That's different from what it does for a
> shared buffer, and different from LocalBufferAlloc(). That's pretty harmless,
> just causes the usage count to be bumped more frequently, but I don't think
> it was intentional. The ordering of bumping the usage count, the local ref
> count, and registration in the resource owner are different too. As far as I
> can see, that makes no difference, but I think we should keep this code as
> close as possible to similar code used elsewhere, unless there's a particular
> reason to differ.
>
> I propose the attached to fix those things.
>
> I tested this by adding this little snippet to a random place where we have
> just read a page with ReadBuffer:
>
> diff --git a/src/backend/access/heap/heapam.c
> b/src/backend/access/heap/heapam.c
> index aab8d6fa4e5..c4abdbc96dd 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -403,6 +403,14 @@ heapgetpage(TableScanDesc sscan, BlockNumber page)
> RBM_NORMAL, scan->rs_strategy);
>scan->rs_cblock = page;
>
> + {
> + bool still_ok;
> +
> + still_ok = ReadRecentBuffer(scan->rs_base.rs_rd->rd_locator,
> MAIN_FORKNUM, page, scan->rs_cbuf);
> + Assert(still_ok);
> + ReleaseBuffer(scan->rs_cbuf);
> + }
> +
>if (!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE))
>return;
>
> Without the fix, the assertion is fails quickly on "make check".
>
> - Heikki<0001-Fix-ReadRecentBuffer-for-local-buffers.patch>
Re: optimize lookups in snapshot [sub]xip arrays
Got it, thanks. Regards, Zhang Mingli > On Jul 25, 2022, at 12:08, Nathan Bossart wrote: > > On Sun, Jul 24, 2022 at 12:48:25PM +0800, Zhang Mingli wrote: >> If snaphost->suboverflowed is false then the subxcnt must be less than >> PGPROC_MAX_CACHED_SUBXIDS which is 64 now. >> >> And we won’t use hash if the xcnt is less than XIP_HASH_MIN_ELEMENTS which >> is 128 currently during discussion. >> >> So that, subxid’s hash table will never be used, right? > > This array will store up to TOTAL_MAX_CACHED_SUBXIDS transactions, which > will typically be much greater than 64. When there isn't any overflow, > subxip stores all of the subxids for all of the entries in the procarray. > > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com
Re: optimize lookups in snapshot [sub]xip arrays
> On Jul 26, 2022, at 03:04, Nathan Bossart wrote: >> > From the discussion thus far, it seems there is interest in optimizing > [sub]xip lookups, so I'd like to spend some time moving it forward. I > think the biggest open question is which approach to take. Both the SIMD > and hash table approaches seem viable, but I think I prefer the SIMD > approach at the moment (see the last paragraph of quoted text for the > reasons). What do folks think? > > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com > > +1, I’m not familiar with SIMD, will try to review this patch. Regards, Zhang Mingli
[Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO
Hi,FORCE_NOT_NULL and FORCE_NULL are only used when COPY FROM.And copyto.c and copyfrom.c are split in this commit https://github.com/postgres/postgres//commit/c532d15dddff14b01fe9ef1d465013cb8ef186df .There is no need to handle these options when COPY TO. vn-0001-Avoid-to-handle-FORCE_NOT_NULL-FORCE_NULL-options.patch Description: Binary data Regards,Zhang Mingli
Re: Fix annotations nextFullXid
Thanks! Regards, Zhang Mingli > On Jul 26, 2022, at 10:08, Fujii Masao wrote: > > > > On 2022/07/23 14:01, Zhang Mingli wrote: >> Hi, >> VariableCacheData.nextFullXid is renamed to nextXid in commit >> https://github.com/postgres/postgres//commit/fea10a64340e529805609126740a540c8f9daab4 >> >> <https://github.com/postgres/postgres//commit/fea10a64340e529805609126740a540c8f9daab4> >> Fix the annotations for less confusion. > > Thanks for the patch! LGTM. I will commit it. > > Regards, > > -- > Fujii Masao > Advanced Computing Technology Center > Research and Development Headquarters > NTT DATA CORPORATION
Re: [Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO
Hi, all Assertions added. Thanks for review. Regards, Zhang Mingli Sent with a Spark On Jul 27, 2022, 14:37 +0800, Richard Guo , wrote: > > > On Wed, Jul 27, 2022 at 12:55 PM Kyotaro Horiguchi > > wrote: > > > ProcessCopyOptions previously rejects force_quote_all for COPY FROM > > > and copyfrom.c is not even conscious of the option (that is, even no > > > assertion on it). The two options are rejected for COPY TO by the same > > > function so it seems like a thinko of the commit. Getting rid of the > > > code would be good in the view of code coverage and maintenance. > > > > Yeah, ProcessCopyOptions() does have the check for force_notnull and > > force_null whether it is using COPY FROM and whether it is in CSV mode. > > So the codes in copyto.c processing force_notnull/force_null are > > actually dead codes. > > > > > On the otherhand I wonder if it is good that we have assertions on the > > > option values. > > > > Agree. Assertions would be better. > > > > Thanks > > Richard vn-0002-Avoid-to-handle-FORCE_NOT_NULL-FORCE_NULL-options.patch Description: Binary data
Re: Patch to address creation of PgStat* contexts with null parent context
Hi,
On Jul 28, 2022, 21:30 +0800, Reid Thompson ,
wrote:
> Hi,
>
> There are instances where pgstat_setup_memcxt() and
> pgstat_prep_pending_entry() are invoked before the CacheMemoryContext
> has been created. This results in PgStat* contexts being created
> without a parent context. Most easily reproduced/seen in autovacuum
> worker via pgstat_setup_memcxt().
>
> Attached is a patch to address this.
>
> To see the issue one can add a line similar to this to the top of
> MemoryContextCreate() in mcxt.c
> fprintf(stderr, "pid: %d ctxname: %s parent is %s CacheMemoryContext is
> %s\n", MyProcPid, name, parent == NULL ? "NULL" : "not NULL",
> CacheMemoryContext == NULL ? "NULL" : "Not NULL")
> and startup postgres and grep for the above after autovacuum workers
> have been invoked
>
> ...snip...
> pid: 1427756 ctxname: PgStat Pending parent is NULL CacheMemoryContext is NULL
> pid: 1427756 ctxname: PgStat Shared Ref parent is NULL CacheMemoryContext is
> NULL
> ...snip...
>
> or
>
> startup postgres, attach gdb to postgres following child, break at
> pgstat_setup_memcxt and wait for autovacuum worker to start...
>
> ...snip...
> ─── Source
> ─
> 384 AllocSetContextCreateInternal(MemoryContext parent,
> 385 const char *name,
> 386 Size minContextSize,
> 387 Size initBlockSize,
> 388 Size maxBlockSize)
> 389 {
> 390 int freeListIndex;
> 391 Size firstBlockSize;
> 392 AllocSet set;
> 393 AllocBlock block;
> ─── Stack
> ──
> [0] from 0x55b7e4088b40 in AllocSetContextCreateInternal+0 at
> /home/rthompso/src/git/postgres/src/backend/utils/mmgr/aset.c:389
> [1] from 0x55b7e3f41c88 in pgstat_setup_memcxt+2544 at
> /home/rthompso/src/git/postgres/src/backend/utils/activity/pgstat_shmem.c:979
> [2] from 0x55b7e3f41c88 in pgstat_get_entry_ref+2648 at
> /home/rthompso/src/git/postgres/src/backend/utils/activity/pgstat_shmem.c:410
> [3] from 0x55b7e3f420ea in pgstat_get_entry_ref_locked+26 at
> /home/rthompso/src/git/postgres/src/backend/utils/activity/pgstat_shmem.c:598
> [4] from 0x55b7e3f3e2c4 in pgstat_report_autovac+36 at
> /home/rthompso/src/git/postgres/src/backend/utils/activity/pgstat_database.c:68
> [5] from 0x55b7e3e7f267 in AutoVacWorkerMain+807 at
> /home/rthompso/src/git/postgres/src/backend/postmaster/autovacuum.c:1694
> [6] from 0x55b7e3e7f435 in StartAutoVacWorker+133 at
> /home/rthompso/src/git/postgres/src/backend/postmaster/autovacuum.c:1493
> [7] from 0x55b7e3e87367 in StartAutovacuumWorker+557 at
> /home/rthompso/src/git/postgres/src/backend/postmaster/postmaster.c:5539
> [8] from 0x55b7e3e87367 in sigusr1_handler+935 at
> /home/rthompso/src/git/postgres/src/backend/postmaster/postmaster.c:5244
> [9] from 0x7fb02bca7420 in __restore_rt
> [+]
> ─── Threads
>
> [1] id 1174088 name postgres from 0x55b7e4088b40 in
> AllocSetContextCreateInternal+0 at
> /home/rthompso/src/git/postgres/src/backend/utils/mmgr/aset.c:389
> ─── Variables
> ──
> arg parent = 0x0, name = 0x55b7e416f179 "PgStat Shared Ref": 80 'P',
> minContextSize = 0, initBlockSize = 1024, maxBlockSize = 8192
> loc firstBlockSize = , set = , block =
> , __FUNCTION__ = "AllocSetContextCreateInternal", __func__ =
> "AllocSetContextCreateInternal"
>
> > > > print CacheMemoryContext == NULL
> $4 = 1
> > > > print parent
> $5 = (MemoryContext) 0x0
>
> Thanks,
> Reid
>
>
Codes seem good, my question is:
Do auto vacuum processes need CacheMemoryContext?
Is it designed not to create CacheMemoryContext in such processes?
If so, we’d better use TopMemoryContext in such processes.
Regards,
Zhang Mingli
Re: COPY FROM FORMAT CSV FORCE_NULL(*) ?
Hi, Agree, FORCE_NULL(*) is useful as well as FORCE_NOT_NULL(*). We can have them both. They are useful when users copy tables that have many columns. Regards, Zhang Mingli On Jul 25, 2022, 21:28 +0800, Andrew Dunstan , wrote: > > On 2022-07-25 Mo 00:18, jian he wrote: > > Hi, there. > > > > copy force null git commit > > <https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3b5e03dca2afea7a2c12dbc8605175d0568b> > > didn't attach a discussion link. So I don't know if it's already been > > discussed. > > > > Current seem you cannot do > > COPY forcetest FROM STDIN WITH (FORMAT csv, FORCE_NULL(*)); > > > > can we have FORCE_NULL(*)? Since We already have FORCE_QUOTE(*). > > > > We only started adding discussion links in later years. Here's a link to > the original discussion. > > > <https://www.postgresql.org/message-id/flat/CAB8KJ%3DjS-Um4TGwenS5wLUfJK6K4rNOm_V6GRUj%2BtcKekL2%3DGQ%40mail.gmail.com> > > > Offhand I don't see why we shouldn't have this. Someone interested > enough would need to submit a patch. > > > cheers > > > andrew > > > -- > Andrew Dunstan > EDB: https://www.enterprisedb.com > > >
Re: [Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO
HI, More assertions added. Thanks. Regards, Zhang Mingli On Jul 29, 2022, 11:24 +0800, Richard Guo , wrote: > > > On Thu, Jul 28, 2022 at 9:04 PM Zhang Mingli wrote: > > > Assertions added. > > > > Can we also add assertions to make sure force_quote, force_notnull and > > force_null are available only in CSV mode? > > > > Thanks > > Richard vn-0003-Avoid-to-handle-FORCE_NOT_NULL-FORCE_NULL-options.patch Description: Binary data
[feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
Hi, The previous discussion is: https://www.postgresql.org/message-id/CACJufxEnVqzOFtqhexF2%2BAwOKFrV8zHOY3y%3Dp%2BgPK6eB14pn_w%40mail.gmail.com We have FORCE_NULL/FORCE_NOT_NULL options when COPY FROM, but users must set the columns one by one. CREATE TABLE forcetest ( a INT NOT NULL, b TEXT NOT NULL, c TEXT, d TEXT, e TEXT ); \pset null NULL BEGIN; COPY forcetest (a, b, c, d) FROM STDIN WITH (FORMAT csv, FORCE_NOT_NULL(c,d), FORCE_NULL(c,d)); 1,'a',,"" \. COMMIT; SELECT c, d FROM forcetest WHERE a = 1; c | d ---+-- | NULL (1 row) We don’t have FORCE_NULL * or FORCE_NOT_NULL * for all columns of a table like FORCE_QUOTE *. They should be helpful if a table have many columns. This patch enables FORCE_NULL/FORCE_NOT_NULL options to select all columns of a table just like FORCE_QUOTE * (quote all columns). BEGIN COPY forcetest (a, b, c, d) FROM STDIN WITH (FORMAT csv, FORCE_NOT_NULL *, FORCE_NULL *); 2,'b',,"" \. COMMIT; SELECT c, d FROM forcetest WHERE a = 2; c | d ---+-- | NULL (1 row) Any thoughts? Regards, Zhang Mingli v0001-COPY-FROM-enable-FORCE_NULL-FORCE_NOT_NULL-on-all-columns.patch Description: Binary data
Re: [Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO
Regards, Zhang Mingli On Aug 2, 2022, 12:30 +0800, Kyotaro Horiguchi , wrote: > An empty List is not NULL but NIL (which values are identical, > though). Thanks for pointing that out. Fix it in new patch. > There are some other option combinations that are rejected > by ProcessCopyOptions. On the other hand *re*checking all > combinations that the function should have rejected is kind of silly. > Addition to that, I doubt the assertions are really needed even though > the wrong values don't lead to any serious consequence. > Agree. ProcessCopyOptions has rejected all invalid combinations and assertions are optional. > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center vn-0004-Avoid-to-handle-FORCE_NOT_NULL-FORCE_NULL-options.patch Description: Binary data
Re: COPY TO (FREEZE)?
Regards, Zhang Mingli On Aug 2, 2022, 12:30 +0800, Kyotaro Horiguchi , wrote: > I noticed that COPY TO accepts FREEZE option but it is pointless. > > Don't we reject that option as the first-attached does? I +1, should be rejected like other invalid options. > > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center
[Code Comments]enum COPY_NEW_FE is removed
Hi, Enum COPY_NEW_FE is removed in commit 3174d69fb9. Should use COPY_FRONTEND instead. Issue exists on 15 and master. ``` typedef struct CopyFromStateData - StringInfo fe_msgbuf; /* used if copy_src == COPY_NEW_FE */ + StringInfo fe_msgbuf; /* used if copy_src == COPY_FRONTEND */ ``` Regards, Zhang Mingli v-0001-Fix-code-comments-COPY_FRONTEND.patch Description: Binary data
Re: [Code Comments]enum COPY_NEW_FE is removed
Ok, thanks. Michael Paquier 于2022年8月6日 周六20:17写道： > On Sat, Aug 06, 2022 at 07:20:25PM +0800, Zhang Mingli wrote: > > Enum COPY_NEW_FE is removed in commit 3174d69fb9. > > > > Should use COPY_FRONTEND instead. > > > > Issue exists on 15 and master. > > This also exists in REL_14_STABLE. I have fixed that on HEAD, as > that's just a comment. > -- > Michael >
Re: Allocator sizeof operand mismatch (src/backend/regex/regcomp.c)
I think it’s ok, re_guts is converted when used (struct guts *) re->re_guts; And there is comments in regex.h char *re_guts; /* `char *' is more portable than `void *' */ Regards, Zhang Mingli On Aug 6, 2022, 20:13 +0800, Ranier Vilela , wrote: > Hi, > > About the error: > Result of 'malloc' is converted to a pointer of type 'char', which is > incompatible with sizeof operand type 'struct guts' > > The patch attached tries to fix this. > > regards, > Ranier Vilela
Re: Allocator sizeof operand mismatch (src/backend/regex/regcomp.c)
On Aug 6, 2022, 22:47 +0800, Tom Lane , wrote: > Zhang Mingli writes: > > I think it’s ok, re_guts is converted when used > > (struct guts *) re->re_guts; > > And there is comments in regex.h > > char *re_guts; /* `char *' is more portable than `void *' */ > > Boy, that comment is showing its age isn't it? If we were to do > anything about this, I'd be more inclined to change re_guts to void*. Got it , thanks.
Re: A cost issue in ORDER BY + LIMIT
HI, What if the the rows of t1 is less than the limit number(ex: t1 has 5 rows, limit 10)? Does it matter? Regards, Zhang Mingli On Aug 6, 2022, 23:38 +0800, Paul Guo , wrote: > > limit_tuples
[patch]HashJoin crash
Hi,
I got a coredump when using hash join on a Postgres derived Database(Greenplum
DB).
And I find a way to reproduce it on Postgres.
Root cause:
In ExecChooseHashTableSize(), commit b154ee63bb uses func pg_nextpower2_size_t
whose param must not be 0.
```
sbuckets = pg_nextpower2_size_t(hash_table_bytes / bucket_size);
```
There is a potential risk that hash_table_bytes < bucket_size in some corner
cases.
Reproduce sql:
```
--create a wide enough table to reproduce the bug
DO language 'plpgsql'
$$
DECLARE var_sql text := 'CREATE TABLE t_1600_columns('
|| string_agg('field' || i::text || ' varchar(255)', ',') || ');'
FROM generate_series(1,1600) As i;
BEGIN
EXECUTE var_sql;
END;
$$ ;
create table j1(field1 text);
set work_mem = 64;
set hash_mem_multiplier = 1;
set enable_nestloop = off;
set enable_mergejoin = off;
explain select * from j1 inner join t_1600_columns using(field1);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded
```
Part of core dump file:
```
#0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=139769161559104)
at ./nptl/pthread_kill.c:44
#1 __pthread_kill_internal (signo=6, threadid=139769161559104) at
./nptl/pthread_kill.c:78
#2 __GI___pthread_kill (threadid=139769161559104, signo=signo@entry=6) at
./nptl/pthread_kill.c:89
#3 0x7f1e8b3de476 in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#4 0x7f1e8b3c47f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x558cc8884062 in ExceptionalCondition (conditionName=0x558cc8a21570
"num > 0 && num <= PG_UINT64_MAX / 2 + 1",
errorType=0x558cc8a21528 "FailedAssertion", fileName=0x558cc8a21500
"../../../src/include/port/pg_bitutils.h", lineNumber=165) at assert.c:69
#6 0x558cc843bb16 in pg_nextpower2_64 (num=0) at
../../../src/include/port/pg_bitutils.h:165
#7 0x558cc843d13a in ExecChooseHashTableSize (ntuples=100, tupwidth=825086,
useskew=true, try_combined_hash_mem=false, parallel_workers=0,
space_allowed=0x7ffdcfa01598, numbuckets=0x7ffdcfa01588,
numbatches=0x7ffdcfa0158c, num_skew_mcvs=0x7ffdcfa01590) at nodeHash.c:835
```
This patch fixes it easily:
```
- sbuckets = pg_nextpower2_size_t(hash_table_bytes / bucket_size);
+ if (hash_table_bytes < bucket_size)
+ sbuckets = 1;
+ else
+ sbuckets = pg_nextpower2_size_t(hash_table_bytes / bucket_size);
```
Or, we could report an error/hit message to tell users to increase
work_mem/hash_mem_multiplier.
And I think let it work is better.
The issue exists on master, 15, 14, 13.
Regards,
Zhang Mingli
vn-0001-Fix-HashJoin-crash.patch
Description: Binary data
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
HI,
Regards,
Zhang Mingli
On Jul 7, 2023, 18:00 +0800, Damir Belyalov , wrote:
>
> The patch does not work for the current version of postgres, it needs to be
> updated.
> I tested your patch. Everything looks simple and works well.
>
> There is a suggestion to simplify the code: instead of using
>
> if (cstate->opts.force_notnull_all)
> {
> int i;
> for(i = 0; i < num_phys_attrs; i++)
> cstate->opt.force_notnull_flags[i] = true;
> }
Thanks very much for review.
Nice suggestion, patch rebased and updated.
v2-0001-COPY-FROM-enable-FORCE_NULL-FORCE_NOT_NULL-on-all-co.patch
Description: Binary data
Re: Improve heapgetpage() performance, overhead from serializable
Hi,
Regards,
Zhang Mingli
On Jul 16, 2023 at 09:57 +0800, Andres Freund , wrote:
> Hi,
>
> Several loops which are important for query performance, like heapgetpage()'s
> loop over all tuples, have to call functions like
> HeapCheckForSerializableConflictOut() and PredicateLockTID() in every
> iteration.
>
> When serializable is not in use, all those functions do is to to return. But
> being situated in a different translation unit, the compiler can't inline
> (without LTO at least) the check whether serializability is needed. It's not
> just the function call overhead that's noticable, it's also that registers
> have to be spilled to the stack / reloaded from memory etc.
>
> On a freshly loaded pgbench scale 100, with turbo mode disabled, postgres
> pinned to one core. Parallel workers disabled to reduce noise. All times are
> the average of 15 executions with pgbench, in a newly started, but prewarmed
> postgres.
>
> SELECT * FROM pgbench_accounts OFFSET 1000;
> HEAD:
> 397.977
>
> removing the HeapCheckForSerializableConflictOut() from heapgetpage()
> (incorrect!), to establish the baseline of what serializable costs:
> 336.695
>
> pulling out CheckForSerializableConflictOutNeeded() from
> HeapCheckForSerializableConflictOut() in heapgetpage(), and avoiding calling
> HeapCheckForSerializableConflictOut() in the loop:
> 339.742
>
> moving the loop into a static inline function, marked as pg_always_inline,
> called with static arguments for always_visible, check_serializable:
> 326.546
>
> marking the always_visible, !check_serializable case likely():
> 322.249
>
> removing TestForOldSnapshot() calls, which we pretty much already decided on:
> 312.987
>
>
> FWIW, there's more we can do, with some hacky changes I got the time down to
> 273.261, but the tradeoffs start to be a bit more complicated. And 397->320ms
> for something as core as this, is imo worth considering on its own.
>
>
>
>
> Now, this just affects the sequential scan case. heap_hot_search_buffer()
> shares many of the same pathologies. I find it a bit harder to improve,
> because the compiler's code generation seems to switch between good / bad with
> changes that seems unrelated...
>
>
> I wonder why we haven't used PageIsAllVisible() in heap_hot_search_buffer() so
> far?
>
>
> Greetings,
>
> Andres Freund
LGTM and I have a fool question:
if (likely(all_visible))
{
if (likely(!check_serializable))
scan->rs_ntuples = heapgetpage_collect(scan, snapshot,
page, buffer,
block, lines, 1, 0);
else
scan->rs_ntuples = heapgetpage_collect(scan, snapshot,
page, buffer,
block, lines, 1, 1);
}
else
{
if (likely(!check_serializable))
scan->rs_ntuples = heapgetpage_collect(scan, snapshot,
page, buffer,
block, lines, 0, 0);
else
scan->rs_ntuples = heapgetpage_collect(scan, snapshot,
page, buffer,
block, lines, 0, 1);
Does it make sense to combine if else condition and put it to the incline
function’s param?
Like:
scan->rs_ntuples = heapgetpage_collect(scan, snapshot, page, buffer,
block, lines, all_visible, check_serializable);
Re: ObjectIdGetDatum() missing from SearchSysCache*() callers
Hi Regards, Zhang Mingli On Jul 17, 2023 at 19:10 +0800, Michael Paquier , wrote: > Hi all, > > While scanning the code, I have noticed that a couple of code paths > that do syscache lookups are passing down directly Oids rather than > Datums. I think that we'd better be consistent here, even if there is > no actual bug. > > I have noticed 11 callers of SearchSysCache*() that pass down > an Oid instead of a Datum. > > Thoughts or comments? > -- > Michael LGTM, and there are two functions missed, in sequence_options pgstuple = SearchSysCache1(SEQRELID, relid); Shall we fix that too?
Re: ObjectIdGetDatum() missing from SearchSysCache*() callers
Hi, Regards, Zhang Mingli On Jul 17, 2023 at 21:09 +0800, Zhang Mingli , wrote: > sequence_options And inside pg_sequence_parameters: pgstuple = SearchSysCache1(SEQRELID, relid);
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
Hi,
On Jul 9, 2023 at 11:51 +0800, Zhang Mingli , wrote:
HI,
Regards,
Zhang Mingli
On Jul 7, 2023, 18:00 +0800, Damir Belyalov , wrote:
The patch does not work for the current version of postgres, it needs to be
updated.
I tested your patch. Everything looks simple and works well.
There is a suggestion to simplify the code: instead of using
if (cstate->opts.force_notnull_all)
{
int i;
for(i = 0; i < num_phys_attrs; i++)
cstate->opt.force_notnull_flags[i] = true;
}
Thanks very much for review.
Nice suggestion, patch rebased and updated.
V2 patch still have some errors when apply file doc/src/sgml/ref/copy.sgml,
rebased and fixed it in V3 path.
Thanks a lot for review.
Zhang Mingli
www.hashdata.xyz
>
v3-0001-COPY-FROM-enable-FORCE_NULL-FORCE_NOT_NULL-on-all-co.patch
Description: Binary data
Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb
HI, On Jun 29, 2023 at 13:24 +0800, Nathan Bossart , wrote: > > Connecting to different databases with the same > host/port/user information seems okay. Have a look, yeah, cluster_all_databases/vacuum_all_databases/reindex_all_databases will get there. LGTM. Regards, Zhang Mingli
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
Hi, On Jul 7, 2023 at 18:00 +0800, Damir Belyalov , wrote: > > V2 patch still have some errors when apply file doc/src/sgml/ref/copy.sgml, > rebased and fixed it in V3 path. > Thanks a lot for review. I have updated https://commitfest.postgresql.org/43/3896/ to staus Ready for Committer, thanks again. Zhang Mingli www.hashdata.xyz
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
HI,
> I've looked at this patch and it looks mostly fine, though I do not
> intend to commit it myself; perhaps Andrew will.
HI, Amit, thanks for review.
>
> A few minor things to improve:
>
> + If * is specified, it will be applied in all
> columns.
> ...
> + If * is specified, it will be applied in all
> columns.
>
> Please write "it will be applied in" as "the option will be applied to".
+1
>
> + boolforce_notnull_all; /* FORCE_NOT_NULL * */
> ...
> + boolforce_null_all; /* FORCE_NULL * */
>
> Like in the comment for force_quote, please add a "?" after * in the
> above comments.
+1
>
> + if (cstate->opts.force_notnull_all)
> + MemSet(cstate->opts.force_notnull_flags, true, num_phys_attrs
> * sizeof(bool));
> ...
> + if (cstate->opts.force_null_all)
> + MemSet(cstate->opts.force_null_flags, true, num_phys_attrs *
> sizeof(bool));
>
> While I am not especially opposed to using this 1-line variant to set
> the flags array, it does mean that there are now different styles
> being used for similar code, because force_quote_flags uses a for
> loop:
>
>if (cstate->opts.force_quote_all)
>{
>int i;
>
>for (i = 0; i < num_phys_attrs; i++)
>cstate->opts.force_quote_flags[i] = true;
>}
>
> Perhaps we could fix the inconsistency by changing the force_quote_all
> code to use MemSet() too. I'll defer whether to do that to Andrew's
> judgement.
Sure, let’s wait for Andrew and I will put everything in one pot then.
Zhang Mingli
https://www.hashdata.xyz
Re: [PATCH] Check more invariants during syscache initialization
HI, > On Jul 26, 2023, at 20:50, Aleksander Alekseev > wrote: > > Hi Michael, > >> That was more a question. I was wondering if it was something you've >> noticed while working on a different patch because you somewhat >> assigned incorrect values in the syscache array, but it looks like you >> have noticed that while scanning the code. > > Oh, got it. That's correct. > >> Still it's hard to miss at compile time. I think that I would remove >> this one. > > Fair enough. Here is the updated patch. > > -- > Best regards, > Aleksander Alekseev > LGTM. ``` - Assert(cacheinfo[cacheId].reloid != 0); + Assert(cacheinfo[cacheId].reloid != InvalidOid); ``` That remind me to have a look other codes, and a grep search `oid != 0` show there are several files using old != 0. ``` .//src/bin/pg_resetwal/pg_resetwal.c: if (set_oid != 0) .//src/bin/pg_resetwal/pg_resetwal.c: if (set_oid != 0) .//src/bin/pg_dump/pg_backup_tar.c: if (oid != 0) .//src/bin/pg_dump/pg_backup_custom.c: while (oid != 0) .//src/bin/pg_dump/pg_backup_custom.c: while (oid != 0) ``` That is another story…I would like provide a patch if it worths. Zhang Mingli https://www.hashdata.xyz
Re: [PATCH] Check more invariants during syscache initialization
Hi, > On Jul 27, 2023, at 09:05, Michael Paquier wrote: > > One reason is that this increases the odds of > conflicts when backpatching on a stable branch. Agree. We could suggest to use OidIsValid() for new patches during review. Zhang Mingli https://www.hashdata.xyz
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
On Aug 1, 2023, at 03:35, Andrew Dunstan wrote:I was hoping it be able to get to it today but that's not happening. If you want to submit a revised patch as above that will be good. I hope to get to it later this week.HI, Andrew Patch rebased and updated like above, thanks. v4-0001-COPY-FROM-enable-FORCE_NULL-FORCE_NOT_NULL-on-all-co.patch Description: Binary data Zhang Minglihttps://www.hashdata.xyz
Fix typo in src/interfaces/libpq/po/zh_CN.po
Hi, The Chinese words there are ok, but the `Unix-domian` should be `Unix-domain`. v1-0001-Fix-typo-src-interfaces-libpq-po-zh_CN.po.patch Description: Binary data Zhang MingliHashData https://www.hashdata.xyz
Re: Fix typo in src/interfaces/libpq/po/zh_CN.po
> On Aug 16, 2023, at 22:24, Peter Eisentraut wrote: > > On 16.08.23 09:34, Zhang Mingli wrote: >> The Chinese words there are ok, but the `Unix-domian` should be >> `Unix-domain`. > > fixed, thanks > Hi, Peter, thanks and just want to make sure that it is pushed? Zhang Mingli HashData https://www.hashdata.xyz
Re: Fix typo in src/interfaces/libpq/po/zh_CN.po
> > This was fixed by Peter as mentioned upthread, but the translations are > maintained in its own Git repository so the commit is not visible in the main > Git repo. Translations are synced with the main repo before releases. The > commit can be seen here: > > https://git.postgresql.org/gitweb/?p=pgtranslation/messages.git;a=commitdiff;h=14391f71ca61e90d52502093447fe1ee0080116f > > -- > Daniel Gustafsson > Thanks, got it~ Zhang Mingli HashData https://www.hashdata.xyz
How to declare GIN index on array type column when bootstrap?
Hi, all I want to add some columns of int(or Oid) array and declare GIN index for it in catalog when bootstrap. But current catalogs all use btree, I tried to declare a GIN index but failed, ex: pg_class.h ``` CATALOG(pg_class ... Int32 my_new_column[1] BKI_DEFAULT(_null_); ... } FormData_pg_class; DECLARE_INDEX(pg_class_my_index, 7200, on pg_class using gin(my_new_column array_ops)); #define ClassMYIndexId 7200 ``` But this failed when init in heap_form_tuple(). I could use SQL to create GIN index column for user tables. But is it possible to declare array column with GIN index in catalog when bootstrap? Thanks. Zhang Mingli www.hashdata.xyz
Fix typo kill_prio_tuple
Hi, Found a typo in mvcc.sql typo kill_prio_tuple -> kill_prior_tuple Regards, Zhang Mingli vn-0001-Fix-typo-kill_prio_tuple-to-kill_prior_tuple.patch Description: Binary data
Remove dead macro exec_subplan_get_plan
Hi, Macro exec_subplan_get_plan is not used anymore. Attach a patch to remove it. Regards, Zhang Mingli vn-0001-exec_subplan_get_plan-is-not-used.patch Description: Binary data
Re: Remove dead macro exec_subplan_get_plan
Hi,all
Regards,
Zhang Mingli
On Sep 6, 2022, 10:22 +0800, Richard Guo , wrote:
>
> On Tue, Sep 6, 2022 at 1:18 AM Tom Lane wrote:
> > Zhang Mingli writes:
> > > Macro exec_subplan_get_plan is not used anymore.
> > > Attach a patch to remove it.
> >
> > Hm, I wonder why it's not used anymore. Maybe we no longer need
> > that list at all? If we do, should use of the macro be
> > re-introduced in the accessors?
The PlannedStmt->subplans list is still used at several places.
> Seems nowadays no one fetches the Plan from PlannedStmt->subplans with a
> certain plan_id any more. Previously back in eab6b8b2 where this macro
> was introduced, it was used in explain_outNode and ExecInitSubPlan.
>
> I find a similar macro, planner_subplan_get_plan, who fetches the Plan
> from glob->subplans. We can use it in the codes where needed. For
> example, in the new function SS_make_multiexprs_unique.
>
> /* Found one, get the associated subplan */
> - plan = (Plan *) list_nth(root->glob->subplans, splan->plan_id - 1);
> + plan = planner_subplan_get_plan(root, splan);
>
> Thanks
> Richard
Yeah, searched on history and found:
exec_subplan_get_plan was once used in ExecInitSubPlan() to create planstate.
```
Plan *plan = exec_subplan_get_plan(estate->es_plannedstmt, subplan);
...
node->planstate = ExecInitNode(plan, sp_estate, eflags);
```
And now in ExecInitSubPlan(), planstate comes from es_subplanstates.
```
/* Link the SubPlanState to already-initialized subplan */
sstate->planstate = (PlanState *) list_nth(estate->es_subplanstates,
subplan->plan_id - 1);
```
And estate->es_subplanstates is evaluated through a for-range of subplans list
at some functions.
```
foreach(l, plannedstmt->subplans)
{
...
estate->es_subplanstates = lappend(estate->es_subplanstates, subplanstate);
}
```
Re: Out-of-date comments about RecentGlobalXmin?
Hi, Regards, Zhang Mingli On Sep 6, 2022, 16:03 +0800, Japin Li , wrote: > > In commit dc7420c2c9, the RecentGlobalXmin variable is removed, however, > there are some places that reference it. > > $ grep 'RecentGlobalXmin' -rn src/ > src/backend/replication/logical/launcher.c:101: * the secondary effect that > it sets RecentGlobalXmin. (This is critical > src/backend/utils/init/postinit.c:790: * interested in the secondary effect > that it sets RecentGlobalXmin. (This > src/backend/postmaster/autovacuum.c:1898: * the secondary effect that it sets > RecentGlobalXmin. (This is critical > Yeah, RecentGlobalXmin is removed. > It's out-of-date, doesn't it? I'm not sure s/RecentGlobalXmin/RecentXmin/g > is right. Any thoughts? I’m afraid not, RecentGlobalXmin is split to several GlobalVis* variables. Need to check one by one.
Re: pg15b4: FailedAssertion("TransactionIdIsValid(xmax)
Hi,
That’s interesting, dig into it for a while but not too much progress.
Maybe we could add some logs to print MultiXactMembers’ xid and status if xid
is 0.
Inside MultiXactIdGetUpdateXid()
```
nmembers = GetMultiXactIdMembers(xmax, &members, false, false);
if (nmembers > 0)
{
int i;
for (i = 0; i < nmembers; i++)
{
/* Ignore lockers */
if (!ISUPDATE_from_mxstatus(members[i].status))
continue;
/* there can be at most one updater */
Assert(update_xact == InvalidTransactionId);
update_xact = members[i].xid;
// log here if xid is invalid
#ifndef USE_ASSERT_CHECKING
/*
* in an assert-enabled build, walk the whole array to ensure
* there's no other updater.
*/
break;
#endif
}
pfree(members);
}
// and here if didn’t update update_xact at all (it shouldn’t happen as
designed)
return update_xact;
```
That will help a little if we can reproduce it.
And could we see multixact reply in logs if db does recover?
Regards,
Zhang Mingli
Removed unused param isSlice of function transformAssignmentSubscripts
Hi, Param isSlice was once used to identity targetTypeId for transformAssignmentIndirection. In commit c7aba7c14e, the evaluation was pushed down to transformContainerSubscripts. No need to keep isSlice around transformAssignmentSubscripts. Attach a patch to remove it. Regards, Zhang Mingli vn-0001-Removed-unused-param-isSlice.patch Description: Binary data
Re: Remove dead macro exec_subplan_get_plan
On Sep 16, 2022, 14:47 +0800, Richard Guo , wrote: > > On Tue, Sep 6, 2022 at 12:39 AM Zhang Mingli wrote: > > Macro exec_subplan_get_plan is not used anymore. > > Attach a patch to remove it. > > How about add it to the CF to not lose track of it? Will add it, thanks~ Regards, Zhang Mingli
Re: Fix typos in code comments
Hi On Sep 19, 2022, 10:57 +0800, Amit Kapila , wrote: > On Mon, Sep 19, 2022 at 8:14 AM houzj.f...@fujitsu.com > wrote: > > > > While working on some other patches, I found serval typos(duplicate words > > and > > incorrect function name reference) in the code comments. Here is a small > > patch > > to fix them. > > > > Thanks, the patch looks good to me. I'll push this. > > -- > With Regards, > Amit Kapila. > > Good catch. There is a similar typo in doc, runtime.sgml. ```using TLS protocols enabled by by setting the parameter```
Free list same_input_transnos in preprocess_aggref
Hi, In preprocess_aggref(), list same_input_transnos is used to track compatible transnos. Free it if we don’t need it anymore. ``` /* * 2. See if this aggregate can share transition state with another * aggregate that we've initialized already. */ transno = find_compatible_trans(root, aggref, shareable, aggtransfn, aggtranstype, transtypeLen, transtypeByVal, aggcombinefn, aggserialfn, aggdeserialfn, initValue, initValueIsNull, same_input_transnos); list_free(same_input_transnos); ``` Not sure if it worths as it will be freed sooner or later when current context ends. But as in find_compatible_agg(), the list is freed if we found a compatible Agg. This patch helps a little when there are lots of incompatible aggs because we will try to find the compatible transnos again and again. Each iteration will keep an unused list memory. Regards, Zhang Mingli vn-0001-free-list-same_input_transnos-in-preprocess_aggref.patch Description: Binary data
Re: Free list same_input_transnos in preprocess_aggref
Regards, Zhang Mingli On Sep 19, 2022, 23:14 +0800, Tom Lane , wrote: > Very little of the planner bothers with freeing small allocations > like that. I think so too, as said, not sure if it worths. > Can you demonstrate a case where this would actually > make a meaningful difference? Offhand, an example may help a little: create table t1(id int); explain select max(id), min(id), sum(id), count(id), avg(id) from t1; Modify codes to test: @@ -139,6 +139,7 @@ preprocess_aggref(Aggref *aggref, PlannerInfo *root) int16 transtypeLen; Oid inputTypes[FUNC_MAX_ARGS]; int numArguments; + static size_t accumulate_list_size = 0; Assert(aggref->agglevelsup == 0); @@ -265,7 +266,7 @@ preprocess_aggref(Aggref *aggref, PlannerInfo *root) aggserialfn, aggdeserialfn, initValue, initValueIsNull, same_input_transnos); - list_free(same_input_transnos); + accumulate_list_size += sizeof(int) * list_length(same_input_transnos); Gdb and print accumulate_list_size for each iteration: SaveBytes = Sum results of accumulate_list_size: 32(4+4+8+8), as we have 5 aggs in sql. If there were N sets of that aggs (more columns as id, with above aggs ), the bytes will be N*SaveBytes. Seems we don’t have so many agg functions that could share the same trans function, Does it worth?
Re: Free list same_input_transnos in preprocess_aggref
Regards, Zhang Mingli On Sep 20, 2022, 00:27 +0800, Zhang Mingli , wrote: > > SaveBytes = Sum results of accumulate_list_size: 32(4+4+8+8), as we have 5 > aggs in sql Correction: SaveBytes = Sum results of accumulate_list_size: 24(4+4+8+8),
Re: Add 64-bit XIDs into PostgreSQL 15
Hi, Is this patch target to PG16 now? I want to have a look at these patches, but apply on master failed: Applying: Use 64-bit numbering of SLRU pages. Applying: Use 64-bit format to output XIDs Applying: Use 64-bit FullTransactionId instead of Epoch:xid Applying: Use 64-bit pages representation in SLRU callers. error: patch failed: src/backend/access/transam/multixact.c:1228 error: src/backend/access/transam/multixact.c: patch does not apply Patch failed at 0004 Use 64-bit pages representation in SLRU callers. hint: Use 'git am --show-current-patch=diff' to see the failed patch Regards, Zhang Mingli
Re: Add 64-bit XIDs into PostgreSQL 15
Hi,
With these patches, it seems that we don’t need to handle wraparound in
GetNextLocalTransactionId() too, as LocalTransactionId is unit64 now.
```
LocalTransactionId
GetNextLocalTransactionId(void)
{
LocalTransactionId result;
/* loop to avoid returning InvalidLocalTransactionId at wraparound */
do
{
result = nextLocalTransactionId++;
} while (!LocalTransactionIdIsValid(result));
return result;
}
```
Regards,
Zhang Mingli
Re: Add 64-bit XIDs into PostgreSQL 15
Hi, On Sep 20, 2022, 17:26 +0800, Justin Pryzby , wrote: > On Tue, Sep 20, 2022 at 03:37:47PM +0800, Zhang Mingli wrote: > > I want to have a look at these patches, but apply on master failed: > > Yeah, it's likely to break every week or more often. > > You have a few options: > > 0) resolve the conflict yourself; > > 1) apply the patch to the commit that the authors sent it against, or > some commit before the conflicting file(s) were changed in master. Like > maybe "git checkout -b 64bitxids f66d997fd". > > 2) Use the last patch that cfbot successfully created. You can read the > patch on github's web interface, or add cfbot's user as a remote to use > the patch locally for review and/or compilation. Something like "git > remote add cfbot https://github.com/postgresql-cfbot/postgresql; git > fetch cfbot commitfest/39/3594; git checkout -b 64bitxids > cfbot/commitfest/39/3594". (Unfortunately, cfbot currently squishes the > patch series into a single commit and loses the commit message). > > You could also check the git link in the commitfest, to see if the > author has already rebased it, but haven't yet mailed the rebased patch > to the list. In this case, that's not true, but you could probably use > the author's branch on github, too. > https://commitfest.postgresql.org/39/3594/ > > -- > Justin Got it, thanks. Regards, Zhang Mingli
Re: Summary function for pg_buffercache
Hi, Correct me if I’m wrong. The doc says we don’t take lock during pg_buffercache_summary, but I see locks in the v8 patch, Isn’t it? ``` Similar to pg_buffercache_pages function pg_buffercache_summary doesn't take buffer manager locks, thus the result is not consistent across all buffers. This is intentional. The purpose of this function is to provide a general idea about the state of shared buffers as fast as possible. Additionally, pg_buffercache_summary allocates much less memory. ``` Regards, Zhang Mingli On Sep 20, 2022, 20:10 +0800, Melih Mutlu , wrote: > Hi, > > Seems like cfbot tests are passing now: > https://cirrus-ci.com/build/4727923671302144 > > Best, > Melih > > > Melih Mutlu , 20 Eyl 2022 Sal, 14:00 tarihinde şunu > > yazdı: > > > Aleksander Alekseev , 20 Eyl 2022 Sal, 13:57 > > > tarihinde şunu yazdı: > > > > > There was a missing empty line in pg_buffercache.out which made the > > > > > tests fail. Here is a corrected v8 patch. > > > > > > > > I was just sending a corrected patch without the missing line. > > > > > > > > Thanks a lot for all these reviews and the corrected patch. > > > > > > > > Best, > > > > Melih
Re: Summary function for pg_buffercache
Hi, Regards, Zhang Mingli On Sep 20, 2022, 20:43 +0800, Aleksander Alekseev , wrote: > > Correct, the procedure doesn't take the locks of the buffer manager. > It does take the locks of every individual buffer. Ah, now I get it, thanks.
Re: Summary function for pg_buffercache
Hi, On Sep 20, 2022, 20:49 +0800, Melih Mutlu , wrote: > Hi Zhang, > > Those are two different locks. > The locks that are taken in the patch are for buffer headers. This locks only > the current buffer and makes that particular buffer's info consistent within > itself. > > However, the lock mentioned in the doc is for buffer manager which would > prevent changes on any buffer if it's held. > pg_buffercache_summary (and pg_buffercache_pages) does not hold buffer > manager lock. Therefore, consistency across all buffers is not guaranteed. > > For pg_buffercache_pages, self-consistent buffer information is useful since > it shows each buffer separately. > > For pg_buffercache_summary, even self-consistency may not matter much since > results are aggregated and we can't see individual buffer information. > Consistency across all buffers is also not a concern since its purpose is to > give an overall idea about the state of buffers. > > I see that these two different locks in the same context can be confusing. I > hope it is a bit more clear now. > > Best, > Melih Thanks for your explanation, LGTM.
Re: Virtual tx id
HI, On Sep 21, 2022, 11:59 +0800, James Sewell , wrote: > Hello Hackers! > > Is it possible to get the current virtual txid from C somehow? > > I've looked through the code, but can't seem to find anything other than > getting a NULL when there is no (real) xid assigned. Maybe I'm missing > something? > > Cheers, > James Virtual xid is meaningful only inside a read-only transaction. It’s made up of MyProc->BackendId and MyProc->LocalTransactionId. To catch it, you can begin a transaction, but don’t exec sqls that could change the db. And gdb on process to see( must exec a read only sql to call StartTransaction, else MyProc->lxid is not assigned). ``` Begin; // do nothing ``` gdb on it and see ``` p MyProc->lxid p MyProc->backendId ``` Regards, Zhang Mingli
Re: Removing unused parameter in SnapBuildGetOrBuildSnapshot
On Sep 21, 2022, 22:22 +0800, Melih Mutlu , wrote: > Hi hackers, > > Sharing a small patch to remove an unused parameter in > SnapBuildGetOrBuildSnapshot function in snapbuild.c > > With commit 6c2003f8a1bbc7c192a2e83ec51581c018aa162f, SnapBuildBuildSnapshot > no longer needs transaction id. This also makes the xid parameter in > SnapBuildGetOrBuildSnapshot useless. > I couldn't see a reason to keep it and decided to remove it. > > Regards, > Melih +1, Good Catch. Regards, Zhang Mingli
