Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
Hello Justin,
Happy new year!
I think the 2nd half of the patches are still waiting for fixes to lstat() on
windows.
Not your problem?
Here is my review about v32:
All patches apply cleanly.
# part 01
One liner doc improvement to tell that creation time is only available on
windows.
It is indeed not available on Linux.
OK.
# part 02
Add tests for various options on pg_ls_dir, and for pg_stat_file, which were not
exercised before. "make check" is ok.
OK.
# part 03
This patch adds a new pg_ls_dir_metadata. Internally, this is an extension of
pg_ls_dir_files function which is used by other pg_ls functions. Doc ok.
About the code:
ISTM that the "if (1) { if (2) continue; } else if(3) { if (4) continue; }"
structure"
may be simplified to "if (1 && 2) continue; if (3 && 4) continue;", at least if
IS_DIR and IS_REG are incompatible? Otherwise, at least "else if (3 & 4)
continue"?
The ifdef WIN32 (which probably detects windows 64 bits…) overwrites values[3].
ISTM
it could be reordered so that there is no overwrite, and simpler single
assignements.
#ifndef WIN32
v = ...;
#else
v = ... ? ... : ...;
#endif
New tests are added which check that the result columns are configured as
required,
including error cases.
"make check" is ok.
OK.
# part 04
Add a new "isdir" column to "pg_ls_tmpdir" output. This is a small behavioral
change. I'm ok with it, however I'm unsure why we would not jump directly to
the "type" char column done later in the patch series. ISTM all such functions
should be extended the same way for better homogeneity? That would also impact
"waldir", "archive_status", "logical_*", "replslot" variants. "make check" ok.
OK.
# part 05
This patch applies my previous advice:-) ISTM that parts 4 and 5 should be one
single patch. The test changes show that only waldir has a test. Would it be
possible to add minimal tests to other variants as well? "make check" ok.
I'd consider add such tests with part 02.
OK.
# part 06
This part extends and adds a test for pg_ls_logdir. ISTM that it should
be merged with the previous patches. "make check" is ok.
OK.
# part 07
This part extends pg_stat_file with more date informations.
ISTM that the documentation should be clear about windows vs unix/cygwin
specific
data provided (change/creation).
The code adds a new value_from_stat function to avoid code duplication.
Fine with me.
All pg_ls_*dir functions are impacted. Fine with me.
"make check" is ok.
OK.
# part 08
This part substitutes lstat to stat. Fine with me. "make check" is ok.
I guess that lstat does something under windows even if the concept of
link is somehow different there. Maybe the doc should say so somewhere?
OK.
# part 09
This part switches the added "isdir" to a "type" column. "make check" is ok.
This is a definite improvement.
OK.
# part 10
This part adds a redundant "isdir" column. I do not see the point.
"make check" is ok.
NOT OK.
# part 11
This part adds a recurse option. Why not. However, the true value does not
seem to be tested? "make check" is ok.
My opinion is unclear.
Overall, ignoring part 10, this makes a definite improvement to postgres ls
capabilities. I do not seen any reason why not to add those.
--
Fabien.
Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
Here is my review about v32: I forgot to tell that doc generation for the cumulated changes also works. -- Fabien.
Re: Refactoring the regression tests for more independence
On Fri, Dec 24, 2021 at 05:00:17PM -0500, Tom Lane wrote: > While I've not done so here, I'm tempted to rename > create_function_0 to create_function_c and create_function_3 to > create_function_sql, to give them better-defined charters and > eliminate the confusion with trailing digits for variant files. +1 > (Maybe we should provide a way to run specified test script(s) *without* > invoking the whole schedule first.) +1 ; it can be done later, though. It's nice to be able to get feedback within a few seconds. That supports the idea of writing tests earlier. I guess this may expose some instabilities due to timing of autovacuum (which I'd say is a good thing). If you rearrange the creation of objects, that may provide an opportunity to rename some tables with too-short names, since backpatching would already have conflicts. -- Justin
Index-only scans vs. partially-retrievable indexes
Yesterday I pushed what I thought was a quick fix for bug #17350 [1].
In short, if we have an index that stores both "x" and "f(x)",
where the "x" column can be retrieved in index-only scans but "f(x)"
cannot, it's possible for the planner to generate an IOS plan that
nonetheless tries to read the f(x) index column. The bug report
concerns the case where f(x) is needed in the IOS plan node's targetlist,
and I did fix that --- but I now realize that we still have a problem
with respect to rechecks of the plan node's indexquals. Here's
an example:
regression=# create extension pg_trgm;
CREATE EXTENSION
regression=# create table t(a text);
CREATE TABLE
regression=# create index on t using gist(lower(a) gist_trgm_ops) include (a);
CREATE INDEX
regression=# insert into t values('zed');
INSERT 0 1
regression=# insert into t values('z');
INSERT 0 1
regression=# select * from t where lower(a) like 'z';
a
---
z
(1 row)
That's the correct answer, but we're using a bitmap scan to get it.
If we force an IOS plan:
regression=# set enable_bitmapscan = 0;
SET
regression=# explain select * from t where lower(a) like 'z';
QUERY PLAN
--
Index Only Scan using t_lower_a_idx on t (cost=0.14..28.27 rows=7 width=32)
Index Cond: ((lower(a)) ~~ 'z'::text)
(2 rows)
regression=# select * from t where lower(a) like 'z';
a
---
(0 rows)
That's from a build a few days old. As of HEAD it's even worse;
not only do we fail to return the rows we should, but EXPLAIN says
regression=# explain select * from t where lower(a) like 'z';
QUERY PLAN
--
Index Only Scan using t_lower_a_idx on t (cost=0.14..28.27 rows=7 width=32)
Index Cond: ((NULL::text) ~~ 'z'::text)
(2 rows)
At least this is showing us what's happening: the index recheck condition
sees a NULL for the value of lower(a). That's because it's trying to
get the value of lower(a) out of the index, instead of recomputing it
from the value of a.
AFAICS this has been broken since 9.5 allowed indexes to contain
both retrievable and non-retrievable columns, so it's a bit surprising
that it hasn't been reported before. I suppose that the case was
harder to hit before we introduced INCLUDE columns. The relevant
code actually claims that it's impossible:
/*
* If the index was lossy, we have to recheck the index quals.
* (Currently, this can never happen, but we should support the case
* for possible future use, eg with GiST indexes.)
*/
if (scandesc->xs_recheck)
{
econtext->ecxt_scantuple = slot;
if (!ExecQualAndReset(node->indexqual, econtext))
{
/* Fails recheck, so drop it and loop back for another */
InstrCountFiltered2(node, 1);
continue;
}
}
That comment may have been true when written (it dates to 9.2) but
it's demonstrably not true now; the test case I just gave traverses
this code, and gets the wrong answer.
I don't think there is any way to fix this that doesn't involve
adding another field to structs IndexOnlyScan and IndexOnlyScanState.
We need a version of the indexqual that references the retrievable
index column x and computes f(x) from that, but the indexqual that's
passed to the index AM still has to reference the f(x) index column.
That's annoying from an API stability standpoint. In the back
branches, we can add the new fields at the end to minimize ABI
breakage, but we will still be breaking any extension code that thinks
it knows how to generate an IndexOnlyScan node directly. (But maybe
there isn't any. The Path representation doesn't need to change, so
typical planner extensions should be OK.)
Unless somebody's got a better idea, I'll push forward with making
this happen.
regards, tom lane
[1]
https://www.postgresql.org/message-id/flat/17350-b5bdcf476e5badbb%40postgresql.org
Re: O(n) tasks cause lengthy startups and checkpoints
Hi, On 2021-12-14 20:23:57 +, Bossart, Nathan wrote: > As promised, here is v2. This patch set includes handling for all > four tasks noted upthread. I'd still consider this a work-in- > progress, as I've done minimal testing. At the very least, it should > demonstrate what an auxiliary process approach might look like. This generates a compiler warning: https://cirrus-ci.com/task/5740581082103808?logs=mingw_cross_warning#L378 Greetings, Andres Freund
Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements
Hi, On 2021-12-03 17:03:46 +0300, Andrei Zubkov wrote: > I've attached a new version of a patch. This fails with an assertion failure: https://cirrus-ci.com/task/5567540742062080?logs=cores#L55 Greetings, Andres Freund
Re: [PATCH] Accept IP addresses in server certificate SANs
Hi, On 2021-12-16 01:13:57 +, Jacob Champion wrote: > Attached is a patch for libpq to support IP addresses in the server's > Subject Alternative Names, which would allow admins to issue certs for > multiple IP addresses, both IPv4 and IPv6, and mix them with > alternative DNS hostnames. These addresses are compared bytewise > instead of stringwise, so the client can contact the server via > alternative spellings of the same IP address. This fails to build on windows: https://cirrus-ci.com/task/6734650927218688?logs=build#L1029 [14:33:28.277] network.obj : error LNK2019: unresolved external symbol pg_inet_net_pton referenced in function network_in [c:\cirrus\postgres.vcxproj] Greetings, Andres Freund
Re: daitch_mokotoff module
On 2021-12-21 22:41:18 +0100, Dag Lem wrote: > This is my very first code contribution to PostgreSQL, and I would be > grateful for any advice on how to proceed in order to get the patch > accepted. Currently the tests don't seem to pass on any platform: https://cirrus-ci.com/task/5941863248035840?logs=test_world#L572 https://api.cirrus-ci.com/v1/artifact/task/5941863248035840/regress_diffs/contrib/fuzzystrmatch/regression.diffs Greetings, Andres Freund
Re: Add jsonlog log_destination for JSON server logs
Hi, On 2021-11-10 22:44:49 +0900, Michael Paquier wrote: > Patch 0003 has been heavily reworked, and it would be good to have an > extra pair of eyes on it. So I have switched the CF entry as "Needs > Review" and added my name to the list of authors (originally this > stuff took code portions of own module, as well). The tests don't seem to pass on windows: https://cirrus-ci.com/task/5412456754315264?logs=test_bin#L47 https://api.cirrus-ci.com/v1/artifact/task/5412456754315264/tap/src/bin/pg_ctl/tmp_check/log/regress_log_004_logrotate psql::1: ERROR: division by zero could not open "c:/cirrus/src/bin/pg_ctl/tmp_check/t_004_logrotate_primary_data/pgdata/current_logfiles": The system cannot find the file specified at t/004_logrotate.pl line 87. Greetings, Andres Freund
Re: Index-only scans vs. partially-retrievable indexes
I wrote:
> Unless somebody's got a better idea, I'll push forward with making
> this happen.
Here's a proposed patch for that. I ended up reverting the code
changes of 4ace45677 in favor of an alternative I'd considered
previously, which is to mark the indextlist elements as resjunk
if they're non-retrievable, and then make setrefs.c deal with
not relying on those elements. This avoids the EXPLAIN breakage
I showed, since now we still have the indextlist elements needed
to interpret the indexqual and indexorderby expressions.
0001 is what I propose to back-patch (modulo putting the new
IndexOnlyScan.recheckqual field at the end, in the back branches).
In addition, it seems to me that we can simplify check_index_only()
by reverting b5febc1d1's changes, because we've now been forced to
put in a non-half-baked solution for the problem it addressed.
That's 0002 attached. I'd be inclined not to back-patch that though.
regards, tom lane
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 09f5253abb..60d0d4ad0f 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1746,7 +1746,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
case T_IndexOnlyScan:
show_scan_qual(((IndexOnlyScan *) plan)->indexqual,
"Index Cond", planstate, ancestors, es);
- if (((IndexOnlyScan *) plan)->indexqual)
+ if (((IndexOnlyScan *) plan)->recheckqual)
show_instrumentation_count("Rows Removed by Index Recheck", 2,
planstate, es);
show_scan_qual(((IndexOnlyScan *) plan)->indexorderby,
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index 0754e28a9a..8fee958135 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -214,13 +214,11 @@ IndexOnlyNext(IndexOnlyScanState *node)
/*
* If the index was lossy, we have to recheck the index quals.
- * (Currently, this can never happen, but we should support the case
- * for possible future use, eg with GiST indexes.)
*/
if (scandesc->xs_recheck)
{
econtext->ecxt_scantuple = slot;
- if (!ExecQualAndReset(node->indexqual, econtext))
+ if (!ExecQualAndReset(node->recheckqual, econtext))
{
/* Fails recheck, so drop it and loop back for another */
InstrCountFiltered2(node, 1);
@@ -555,8 +553,8 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
*/
indexstate->ss.ps.qual =
ExecInitQual(node->scan.plan.qual, (PlanState *) indexstate);
- indexstate->indexqual =
- ExecInitQual(node->indexqual, (PlanState *) indexstate);
+ indexstate->recheckqual =
+ ExecInitQual(node->recheckqual, (PlanState *) indexstate);
/*
* If we are just doing EXPLAIN (ie, aren't going to run the plan), stop
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index df0b747883..18e778e856 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -519,6 +519,7 @@ _copyIndexOnlyScan(const IndexOnlyScan *from)
*/
COPY_SCALAR_FIELD(indexid);
COPY_NODE_FIELD(indexqual);
+ COPY_NODE_FIELD(recheckqual);
COPY_NODE_FIELD(indexorderby);
COPY_NODE_FIELD(indextlist);
COPY_SCALAR_FIELD(indexorderdir);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 91a89b6d51..6c0979ec35 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -580,6 +580,7 @@ _outIndexOnlyScan(StringInfo str, const IndexOnlyScan *node)
WRITE_OID_FIELD(indexid);
WRITE_NODE_FIELD(indexqual);
+ WRITE_NODE_FIELD(recheckqual);
WRITE_NODE_FIELD(indexorderby);
WRITE_NODE_FIELD(indextlist);
WRITE_ENUM_FIELD(indexorderdir, ScanDirection);
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index d79af6e56e..2a699c216b 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1884,6 +1884,7 @@ _readIndexOnlyScan(void)
READ_OID_FIELD(indexid);
READ_NODE_FIELD(indexqual);
+ READ_NODE_FIELD(recheckqual);
READ_NODE_FIELD(indexorderby);
READ_NODE_FIELD(indextlist);
READ_ENUM_FIELD(indexorderdir, ScanDirection);
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index ff2b14e880..4b7347bc0e 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -20,7 +20,6 @@
#include
#include "access/sysattr.h"
-#include "catalog/pg_am.h"
#include "catalog/pg_class.h"
#include "foreign/fdwapi.h"
#include "miscadmin.h"
@@ -189,10 +188,10 @@ static IndexScan *make_indexscan(List *qptlist, List *qpqual, Index scanrelid,
ScanDirection indexscandir);
static IndexOnlyScan *make_indexonlyscan(List *qptlist, List *qpqual,
Index scanrelid, Oid indexid,
- List *indexqual, List *indexorderby,
+ List *indexqual, List *recheckqual,
+ List *indexorderby,
List *indextl
Re: daitch_mokotoff module
On Mon, Jan 3, 2022 at 10:32 AM Andres Freund wrote:
> On 2021-12-21 22:41:18 +0100, Dag Lem wrote:
> > This is my very first code contribution to PostgreSQL, and I would be
> > grateful for any advice on how to proceed in order to get the patch
> > accepted.
>
> Currently the tests don't seem to pass on any platform:
> https://cirrus-ci.com/task/5941863248035840?logs=test_world#L572
> https://api.cirrus-ci.com/v1/artifact/task/5941863248035840/regress_diffs/contrib/fuzzystrmatch/regression.diffs
Erm, it looks like something weird is happening somewhere in cfbot's
pipeline, because Dag's patch says:
+SELECT daitch_mokotoff('Straßburg');
+ daitch_mokotoff
+-
+ 294795
+(1 row)
... but it's failing like:
SELECT daitch_mokotoff('Straßburg');
daitch_mokotoff
-
- 294795
+ 297950
(1 row)
It's possible that I broke cfbot when upgrading to Python 3 a few
months back (ie encoding snafu when using the "requests" module to
pull patches down from the archives). I'll try to fix this soon.
Re: daitch_mokotoff module
Thomas Munro writes:
> Erm, it looks like something weird is happening somewhere in cfbot's
> pipeline, because Dag's patch says:
> +SELECT daitch_mokotoff('Straßburg');
> + daitch_mokotoff
> +-
> + 294795
> +(1 row)
... so, that test case is guaranteed to fail in non-UTF8 encodings,
I suppose? I wonder what the LANG environment is in that cfbot
instance.
(We do have methods for dealing with non-ASCII test cases, but
I can't see that this patch is using any of them.)
regards, tom lane
Re: Multi-Column List Partitioning
On Wed, Dec 29, 2021 at 7:26 PM Nitin Jadhav
wrote:
>
>
> > -* For range partitioning, we must only perform pruning with values
> > -* for either all partition keys or a prefix thereof.
> > +* For range partitioning and list partitioning, we must only
> > perform
> > +* pruning with values for either all partition keys or a prefix
> > +* thereof.
> > */
> > - if (keyno > nvalues && context->strategy ==
> > PARTITION_STRATEGY_RANGE)
> > + if (keyno > nvalues && (context->strategy ==
> > PARTITION_STRATEGY_RANGE ||
> > + context->strategy ==
> > PARTITION_STRATEGY_LIST))
> >break;
> >
> > I think this is not true for multi-value list partitions, we might
> > still want prune partitions for e.g. (100, IS NULL, 20). Correct me
> > if I am missing something here.
>
> AFAIK, the above condition/comments says that, either we should
> include all keys or prefixes of the partition keys to get the
> partition pruning results. For example if we have a table with 2
> columns and both are present in the partition key. Let the column
> names be 'a' and 'b'.
>
> SELECT * FROM table WHERE a=1 AND b=1; - This query works for pruning
> and it refers to a comment which says all partition keys are included.
> SELECT * FROM table WHERE b=1; - Here partition pruning does not work
> as it does not contain prefix of the partition keys.
> SELECT * FROM table WHERE a=1; - This query works fine as column 'a'
> is prefix of partition keys.
>
> Please let me know if you need more information.
That what I was assuming is not correct. The dependency of the prefix
is true for the range partitioning but why should that be in the case
of list partitioning? I think all partitioning keys in the list will
not be dependent on each other, AFAICU. If you prune list partitions
based on the b=1 value that still is correct & gives the correct
result, correct me If I am wrong.
> ---
>
> > +/*
> > + * get_min_and_max_offset
> > + *
> > + * Fetches the minimum and maximum offset of the matching partitions.
> > + */
> >
> > ...
> >
> > +/*
> > + * get_min_or_max_off
> > + *
> > + * Fetches either minimum or maximum offset of the matching partitions
> > + * depending on the value of is_min parameter.
> > + */
> >
> > I am not sure we really have to have separate functions but if needed
> > then I would prefer to have a separate function for each min and max
> > rather than combining.
>
> If we don't make a separate function, then we have to include this
> code in get_matching_list_bounds() which is already a big function. I
> just made a separate function to not increase the complexity of
> get_matching_list_bounds() and most of the code present in
> get_min_or_max_off() is common for min and max calculation. If we make
> it separate then there might be a lot of duplications. Please let me
> know if you still feel if any action is required.
Hmm, ok, I personally didn't like to have two functions one gives max
and min and the other gives only max or min, the other could have
different opinions.
How about keeping only one function say, get_min_max_off() and based
on the argument e.g. minoff & maxoff fetch the value, I mean e.g. if
minoff is not null then fetch the value otherwise skip that, same for
maxoff too.
> ---
>
> > + if (part_scheme->strategy != PARTITION_STRATEGY_LIST)
> > + {
> > + *clause_is_not_null = (nulltest->nulltesttype == IS_NOT_NULL);
> > + return PARTCLAUSE_MATCH_NULLNESS;
> > + }
> > +
> > + expr = makeConst(UNKNOWNOID, -1, InvalidOid, -2, (Datum) 0,
> > true, false);
> > + partclause = (PartClauseInfo *) palloc(sizeof(PartClauseInfo));
> > +
> > + partclause->keyno = partkeyidx;
> > + partclause->expr = (Expr *) expr;
> > + partclause->is_null = true;
> > +
> > + if (nulltest->nulltesttype == IS_NOT_NULL)
> > + {
> > + partclause->op_is_ne = true;
> > + partclause->op_strategy = InvalidStrategy;
> > + }
> > + else
> > + {
> > + partclause->op_is_ne = false;
> > + partclause->op_strategy = BTEqualStrategyNumber;
> > + }
> >
> > - return PARTCLAUSE_MATCH_NULLNESS;
> > + *pc = partclause;
> > + return PARTCLAUSE_MATCH_CLAUSE;
> >
> > I still believe considering NULL value for match clause is not a
> > fundamentally correct thing. And that is only for List partitioning
> > which isn't aligned with the other partitioning.
>
> As other partitions which support multiple partition keys (Range
> partitioning) do not support NULL values. This feature supports
> multiple partition keys with list partitioning and it also supports
> NULL values. With the existing design, I have tried to support this
> feature with minimal changes as possible. If this is not the right
> approach to support NULL values, I would like to know how we can
> support multiple NULL values. Kindly pro
Remove inconsistent quotes from date_part error
The attached patch corrects a very minor typographical inconsistency when date_part is invoked with invalid units on time/timetz data vs timestamp/timestamptz/interval data. (If stuff like this is too minor to bother with, let me know and I'll hold off in the future... but since this was pointed out to me I can't unsee it.) Nikhil 0001-Remove-inconsistent-quotes-from-date_part-error.patch Description: Binary data
pg_receivewal fail to streams when the partial file to write is not fully initialized present in the wal receiver directory
Hi Hackers, I noticed that pg_receivewal fails to stream when the partial file to write is not fully initialized and fails with the error message something like below. This requires an extra step of deleting the partial file that is not fully initialized before starting the pg_receivewal. Attaching a simple patch that creates a temp file, fully initialize it and rename the file to the desired wal segment name. "error: write-ahead log file "00010003.partial" has 8396800 bytes, should be 0 or 16777216" Thanks, Satya pg_receivewal_init_fix.patch Description: Binary data
RE: Failed transaction statistics to measure the logical replication progress
On Wednesday, December 22, 2021 6:14 PM Osumi, Takamichi wrote: > Attached the new patch v19. Hi, Thanks for updating the patch. --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -15,6 +15,7 @@ #include "portability/instr_time.h" #include "postmaster/pgarch.h" /* for MAX_XFN_CHARS */ #include "replication/logicalproto.h" +#include "replication/worker_internal.h" I noticed that the patch includes "worker_internal.h " in pgstat.h. I think it might be better to only include this file in pgstat.c. And it seems we can access MyLogicalRepWorker directly in the following functions instead of passing a parameter. +extern void pgstat_report_subworker_xact_end(LogicalRepWorker *repWorker, + LogicalRepMsgType command, + bool bforce); +extern void pgstat_send_subworker_xact_stats(LogicalRepWorker *repWorker, + bool force); Best regards, Hou zj
Use MaxLockMode in lock methods initialization
Hi,
While reading some code around I noticed that b04aeb0a053e7 added a MaxLockMode
but didn't update the lock methods initialization. It shouldn't make much
difference in the long run but some consistency seems better to me.
>From 1a3de43f4ed75466413ffcd884678332960c2520 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Mon, 3 Jan 2022 12:19:11 +0800
Subject: [PATCH] Use MaxLockMode in LockMethodData initialization.
Commit b04aeb0a053e7 added a MaxLockMode to hold the highest lock mode so let's
use it here too.
---
src/backend/storage/lmgr/lock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index c25af7fe09..2e985df5af 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -124,7 +124,7 @@ static bool Dummy_trace = false;
#endif
static const LockMethodData default_lockmethod = {
- AccessExclusiveLock,/* highest valid lock mode number */
+ MaxLockMode,/* highest valid lock mode number */
LockConflicts,
lock_mode_names,
#ifdef LOCK_DEBUG
@@ -135,7 +135,7 @@ static const LockMethodData default_lockmethod = {
};
static const LockMethodData user_lockmethod = {
- AccessExclusiveLock,/* highest valid lock mode number */
+ MaxLockMode,/* highest valid lock mode number */
LockConflicts,
lock_mode_names,
#ifdef LOCK_DEBUG
--
2.33.1
Re: O(n) tasks cause lengthy startups and checkpoints
On Mon, Jan 3, 2022 at 2:56 AM Andres Freund wrote: > > Hi, > > On 2021-12-14 20:23:57 +, Bossart, Nathan wrote: > > As promised, here is v2. This patch set includes handling for all > > four tasks noted upthread. I'd still consider this a work-in- > > progress, as I've done minimal testing. At the very least, it should > > demonstrate what an auxiliary process approach might look like. > > This generates a compiler warning: > https://cirrus-ci.com/task/5740581082103808?logs=mingw_cross_warning#L378 > Somehow, I am not getting these compiler warnings on the latest master head (69872d0bbe6). Here are the few minor comments for the v2 version, I thought would help: + * Copyright (c) 2021, PostgreSQL Global Development Group Time to change the year :) -- + + /* These operations are really just a minimal subset of +* AbortTransaction(). We don't have very many resources to worry +* about. +*/ Incorrect formatting, the first line should be empty in the multiline code comment. -- + XLogRecPtr logical_rewrite_mappings_cutoff;/* can remove older mappings */ + XLogRecPtr logical_rewrite_mappings_cutoff_set; Look like logical_rewrite_mappings_cutoff gets to set only once and never get reset, if it is true then I think that variable can be skipped completely and set the initial logical_rewrite_mappings_cutoff to InvalidXLogRecPtr, that will do the needful. -- Regards, Amul
Clarify planner_hook calling convention
Hi,
planner hook is frequently used in monitoring and advising extensions.
The call to this hook is implemented in the way, that the
standard_planner routine must be called at least once in the hook's call
chain.
But, as I see in [1], it should allow us "... replace the planner
altogether".
In such situation it haven't sense to call standard_planner at all.
Moreover, if an extension make some expensive planning activity,
monitoring tools, like pg_stat_statements, can produce different
results, depending on a hook calling order.
I thought about additional hooks, explicit hook priorities and so on.
But, maybe more simple solution is to describe requirements to such kind
of extensions in the code and documentation (See patch in attachment)?
It would allow an extension developer legally check and log a situation,
when the extension doesn't last in the call chain.
[1]
https://www.postgresql.org/message-id/flat/27516.1180053940%40sss.pgh.pa.us
--
regards,
Andrey Lepikhov
Postgres Professional
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index afbb6c35e3..79a5602850 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -9401,6 +9401,7 @@ SET XML OPTION { DOCUMENT | CONTENT };
double quotes if you need to include whitespace or commas in the name.
This parameter can only be set at server start. If a specified
library is not found, the server will fail to start.
+Libraries are loaded in the order in which they appear in the list.
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index bd01ec0526..7251b88ad1 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -261,8 +261,11 @@ static int common_prefix_cmp(const void *a, const void *b);
* after the standard planning process. The plugin would normally call
* standard_planner().
*
- * Note to plugin authors: standard_planner() scribbles on its Query input,
- * so you'd better copy that data structure if you want to plan more than once.
+ * Notes to plugin authors:
+ * 1. standard_planner() scribbles on its Query input, so you'd better copy that
+ * data structure if you want to plan more than once.
+ * 2. If your extension implements some planning activity, write in the extension
+ * docs a requirement to set the extension at the begining of shared libraries list.
*
*/
PlannedStmt *
Re: Confused comment about drop replica identity index
On Thu, Dec 30, 2021 at 06:45:30AM +, houzj.f...@fujitsu.com wrote: > I think forbids DROP INDEX might not completely solve this problem. Because > user could still use other command to delete the index, for example: ALTER > TABLE DROP COLUMN. After dropping the column, the index on it will also be > dropped. > > Besides, user can also ALTER REPLICA IDENTITY USING INDEX "primary key", and > in > this case, when they ALTER TABLE DROP CONSTR "PRIMARY KEY", the replica > identity index will also be dropped. Indexes related to any other object type, like constraints, are dropped as part of index_drop() as per the handling of dependencies. So, by putting a restriction there, any commands would take this code path, and fail when trying to drop an index used as a replica identity. Why would that be logically a problem? We may want errors with more context for such cases, though, as complaining about an object not directly known by the user when triggering a different command, like a constraint index, could be confusing. -- Michael signature.asc Description: PGP signature
Re: pg_receivewal fail to streams when the partial file to write is not fully initialized present in the wal receiver directory
On Sun, Jan 02, 2022 at 09:27:43PM -0800, SATYANARAYANA NARLAPURAM wrote:
> I noticed that pg_receivewal fails to stream when the partial file to write
> is not fully initialized and fails with the error message something like
> below. This requires an extra step of deleting the partial file that is not
> fully initialized before starting the pg_receivewal. Attaching a simple
> patch that creates a temp file, fully initialize it and rename the file to
> the desired wal segment name.
Are you referring to the pre-padding when creating a new partial
segment, aka when we write chunks of XLOG_BLCKSZ full of zeros until
the file is fully created? What kind of error did you see? I guess
that a write() with ENOSPC would be more likely, but you got a
different problem? I don't disagree with improving such cases, but we
should not do things so as there is a risk of leaving behind an
infinite set of segments in case of repeated errors, and partial
segments are already a kind of temporary file.
- if (dir_data->sync)
+ if (shouldcreatetempfile)
+ {
+ if (durable_rename(tmpsuffixpath, targetpath) != 0)
+ {
+ close(fd);
+ unlink(tmpsuffixpath);
+ return NULL;
+ }
+ }
durable_rename() does a set of fsync()'s, but --no-sync should not
flush any data.
--
Michael
signature.asc
Description: PGP signature
