Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, May 18, 2022 at 03:12:45PM +0800, Julien Rouhaud wrote:
> The cfbot reports that the patch doesn't apply anymore, rebased v7 attached.
+ switch (kind)
+ {
+ case SecondaryAuthFile:
+ msglog = "could not open secondary authentication file \"@%s\"
as \"%s\": %m";
+ msgview = "could not open secondary authentication file \"@%s\"
as \"%s\": %s";
+ break;
+ case IncludedAuthFile:
+ msglog = "could not open included authentication file \"%s\" as
\"%s\": %m";
+ msgview = "could not open included authentication file \"%s\"
as \"%s\": %s";
+ break;
+ default:
+ elog(ERROR, "unknown HbaIncludeKind: %d", kind);
+ break;
+ }
I don't think that HbaIncludeKind is necessary, considering that we
could rely on the file name to provide enough context about the type
involved in the error string generated. The "default" clause in the
switch could just be removed, btw, to generate warnings if a new value
is added in the kind enum.
+ /* relative path is relative to dir of calling file */
There are many relatives here. It seems to me that this is the kind
of behavior we should document precisely (and test!). I am
understanding the following for cascading configurations:
- pg_hba.conf has "include_dir path1".
- path1/ has file1.conf that has "include file2.conf". This means
that file2.conf has to be also included in path1/.
postmaster.c and postinit.c do that:
/*
* Load configuration files for client authentication.
*/
if (!load_hba())
{
/*
* It makes no sense to continue if we fail to load the HBA file,
* since there is no way to connect to the database in this case.
*/
ereport(FATAL,
(errmsg("could not load pg_hba.conf")));
}
This could be confusing as a different file may fail to load, while
pg_hba.conf was able to do its work outside an include clause.
How does include_dir work with the ordering of the HBA entries across
multiple files? The set of HBA rules are very sensitive to their
ordering, and ReadDir() depends on readdir(), which provides a list
of files depending on its FS implementation. That does not seem like
a sane concept to me. I can this this order (tracked by rule_number)
being strictly enforced when it comes to the loading of files, though,
so I would recommend to focus on the implementation of "include"
rather than "include_dir".
+
+ Rule number, in priority order, of this rule if the rule is valid,
+ otherwise null
+
This is a very important field. I think that this explanation should
be extended and explain why relying on this number counts (aka this is
the order of the rules loaded across files). Btw, this could be added
as a separate patch, even if this maps to the line number when it
comes to the ordering.
+# include FILE
+# include_dir FILE
You mean s/FILE/DIRECTORY/ for include_dir, I guess?
+ /*
+* Only parse files with names ending in ".conf".
+* Explicitly reject files starting with ".". This
+* excludes things like "." and "..", as well as typical
+* hidden files, backup files, and editor debris.
+*/
I don't think that there is any need to restrict that to files ending
with .conf. We don't do that for postgresql.conf's include, for one.
In 0002, pg_hba_matches() had better have some documentation,
explaining for which purpose this function can be used with a short
example (aka for an address and a role, find the matching set of HBA
rules and report their line and file)?
I am not sure to be a huge fan of this implementation, actually. The
function is shaped so as the user provides in input the arguments to
fill hbaPort with, passing it down to check_hba(). This could bite
easily in the future if some of the internal fields filled in by the
HBA load and used by the HBA check change over time, particularly if
this stuff has no tests to provide some validation, though we
discussed that a couple of months ago. Perhaps we should think
harder on this point.
+ char
tmp[sizeof("::::::255.255.255.255/128")];
Okay. This is the same way of doing things as network.c or
inet_cidr_ntop.c. Shouldn't we centralize the definition of such a
maximum size instead?
+ if (!load_hba())
+ ereport(ERROR,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+errmsg("Invalidation auth configuration file")));
This error message sounds wrong to me. It would be more consistent to
write that as "could not load authentication file" or such.
postinit.c and postmaster.c do that (these error strings become
partially confusing with the possibility to include extra auth files,
actually, on a separate note).
+ if (PG_ARGISNULL(1))
+ er
Re: postgres_fdw has insufficient support for large object
On Mon, May 23, 2022 at 1:21 PM Tom Lane wrote: > The big picture here is that Postgres is a hodgepodge of features > that were developed at different times and with different quality > standards, over a period that's now approaching forty years. > Some of these features interoperate better than others. Large > objects, in particular, are largely a mess with a lot of issues > such as not having a well-defined garbage collection mechanism. > They do not interoperate well with foreign tables, or several > other things, and you will not find anybody excited about putting > effort into fixing that. We're unlikely to remove large objects > altogether, because some people use them successfully and we're not > about breaking cases that work today. We could possibly have a category of such features and label them "obsolete", where we don't threaten to remove them someday (i.e. "deprecated"), but we are not going to improve them in any meaningful way, and users would be warned about using them in new projects if better alternatives are available. -- John Naylor EDB: http://www.enterprisedb.com
Re: postgres_fdw has insufficient support for large object
"=?gb18030?B?U2FsYWRpbg==?=" writes: > The output i expected: > pg_largeobject_metadata and pg_largeobject in both database A and database > B should have rows.Shouldn't only in database A.So, i can use large object > functions > to operate large_objectin remote table or foreign table. The big picture here is that Postgres is a hodgepodge of features that were developed at different times and with different quality standards, over a period that's now approaching forty years. Some of these features interoperate better than others. Large objects, in particular, are largely a mess with a lot of issues such as not having a well-defined garbage collection mechanism. They do not interoperate well with foreign tables, or several other things, and you will not find anybody excited about putting effort into fixing that. We're unlikely to remove large objects altogether, because some people use them successfully and we're not about breaking cases that work today. But they're fundamentally incompatible with use in foreign tables in the way you expect, and that is not likely to get fixed. regards, tom lane
Re: Add --{no-,}bypassrls flags to createuser
On 2022-05-21 06:45, Nathan Bossart wrote: On Thu, May 19, 2022 at 10:35:23AM +0900, Shinya Kato wrote: I created a new patch to test the new options! Thanks for the new patch! I attached a new version with a few small changes. What do you think? Thanks for updating the patch! It looks good to me. On 2022-05-23 10:32, Kyotaro Horiguchi wrote: Anyway, after fixing that issue we will modify the createrole command so that it uses the new SQL feature. I find no hard obstacles in reaching there in the 16 cycle. +1. -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: A qsort template
I wrote: > I agree this is only useful in development. Removal sounds fine to me, > so I'll do that soon. This is done. -- John Naylor EDB: http://www.enterprisedb.com
Re: automatically generating node support functions
On 25.03.22 14:08, Peter Eisentraut wrote: 2. Some of these comment lines have become pretty long after having added the attribute macro. e.g. PlannerInfo *subroot pg_node_attr(readwrite_ignore); /* modified "root" for planning the subquery; not printed, too large, not interesting enough */ I wonder if you'd be better to add a blank line above, then put the comment on its own line, i.e: /* modified "root" for planning the subquery; not printed, too large, not interesting enough */ PlannerInfo *subroot pg_node_attr(readwrite_ignore); Yes, my idea was to make a separate patch first that reformats many of the structs and comments in that way. Here is a patch that reformats the relevant (and a few more) comments that way. This has been run through pgindent, so the formatting should be stable.From 5eea69417e524779e2e3cc5164966646cb2c2c0e Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Mon, 23 May 2022 07:40:12 +0200 Subject: [PATCH] Reformat node comments --- src/include/nodes/parsenodes.h | 3 +- src/include/nodes/pathnodes.h | 686 ++--- src/include/nodes/plannodes.h | 423 ++-- src/include/nodes/primnodes.h | 166 +--- 4 files changed, 899 insertions(+), 379 deletions(-) diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 73f635b455..f93d866548 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -123,7 +123,8 @@ typedef struct Query QuerySource querySource;/* where did I come from? */ - uint64 queryId;/* query identifier (can be set by plugins) */ + /* query identifier (can be set by plugins) */ + uint64 queryId; boolcanSetTag; /* do I set the command result tag? */ diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index a6e5db4eec..b88cfb8dc0 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -226,8 +226,8 @@ struct PlannerInfo * even when using the hash table for lookups; this simplifies life for * GEQO. */ - List *join_rel_list; /* list of join-relation RelOptInfos */ - struct HTAB *join_rel_hash; /* optional hashtable for join relations */ + List *join_rel_list; + struct HTAB *join_rel_hash; /* * When doing a dynamic-programming-style join search, join_rel_level[k] @@ -329,11 +329,16 @@ struct PlannerInfo */ List *update_colnos; - /* Fields filled during create_plan() for use in setrefs.c */ - AttrNumber *grouping_map; /* for GroupingFunc fixup */ - List *minmax_aggs;/* List of MinMaxAggInfos */ + /* +* Fields filled during create_plan() for use in setrefs.c +*/ + /* for GroupingFunc fixup */ + AttrNumber *grouping_map; + /* List of MinMaxAggInfos */ + List *minmax_aggs; - MemoryContext planner_cxt; /* context holding PlannerInfo */ + /* context holding PlannerInfo */ + MemoryContext planner_cxt; Cardinality total_table_pages; /* # of pages in all non-dummy tables of * query */ @@ -369,9 +374,12 @@ struct PlannerInfo Relids curOuterRels; /* outer rels above current node */ List *curOuterParams; /* not-yet-assigned NestLoopParams */ - /* These fields are workspace for setrefs.c */ - bool *isAltSubplan; /* array corresponding to glob->subplans */ - bool *isUsedSubplan; /* array corresponding to glob->subplans */ + /* +* These fields are workspace for setrefs.c. Each is an array +* corresponding to glob->subplans. +*/ + bool *isAltSubplan; + bool *isUsedSubplan; /* optional private data for join_search_hook, e.g., GEQO */ void *join_search_private; @@ -678,21 +686,37 @@ typedef struct RelOptInfo RelOptKind reloptkind; - /* all relations included in this RelOptInfo */ - Relids relids; /* set of base relids (rangetable indexes) */ + /* +* all relations included in this RelOptInfo; set of base relids +* (rangetable indexes) +*/ + Relids relids; - /* size estimates generated by planner */ - Cardinality rows; /* estimated number of result tuples */ + /* +* size estimates generated by planner +*/ + /* estimated number of result tuples */ + Cardinality rows; - /* per-relation planner control flags */ - boolconsider_startup; /* keep cheap-startup-cost paths? */ - boolconsider_param_startup; /* ditto, for parameterized paths? */
Re: postgres_fdw has insufficient support for large object
On Sunday, May 22, 2022, Saladin wrote: > > The output i expected: > pg_largeobject_metadata and pg_largeobject in both database A and database > B should have rows.Shouldn't only in database A.So, i can use large object > functions > to operate large_objectin remote table or foreign table. > This is an off-topic email for the -hackers mailing list. -general is the appropriate list. Your expectation is simply unsupported by anything in the documentation. If you want to do what you say you will need to use dblink (and the file needs to be accessible to the remote server directly) and directly execute entire queries on the remote server, the FDW infrastructure simply does not work in the way you are expecting. Or just use bytea. David J.
Re: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns
On Mon, May 23, 2022 at 10:03 AM Kyotaro Horiguchi wrote: > > At Sat, 21 May 2022 15:35:58 +0530, Amit Kapila > wrote in > > I think if we don't have any better ideas then we should go with > > either this or one of the other proposals in this thread. The other > > idea that occurred to me is whether we can somehow update the snapshot > > we have serialized on disk about this information. On each > > running_xact record when we serialize the snapshot, we also try to > > purge the committed xacts (via SnapBuildPurgeCommittedTxn). So, during > > that we can check if there are committed xacts to be purged and if we > > have previously serialized the snapshot for the prior running xact > > record, if so, we can update it with the list of xacts that have > > catalog changes. If this is feasible then I think we need to somehow > > remember the point where we last serialized the snapshot (maybe by > > using builder->last_serialized_snapshot). Even, if this is feasible we > > may not be able to do this in back-branches because of the disk-format > > change required for this. > > > > Thoughts? > > I didn't look it closer, but it seems to work. I'm not sure how much > spurious invalidations at replication start impacts on performance, > but it is promising if the impact is significant. > It seems Sawada-San's patch is doing at each commit not at the start of replication and I think that is required because we need this each time for replication restart. So, I feel this will be an ongoing overhead for spurious cases with the current approach. > That being said I'm > a bit negative for doing that in post-beta1 stage. > Fair point. We can use the do it early in PG-16 if the approach is feasible, and backpatch something on lines of what Sawada-San or you proposed. -- With Regards, Amit Kapila.
Re: Convert macros to static inline functions
On 16.05.22 15:23, Amul Sul wrote:
+static inline OffsetNumber
+PageGetMaxOffsetNumber(Page page)
+{
+ if (((PageHeader) page)->pd_lower <= SizeOfPageHeaderData)
+ return 0;
+ else
+ return PageHeader) page)->pd_lower - SizeOfPageHeaderData)
/ sizeof(ItemIdData));
+}
The "else" is not necessary, we can have the return statement directly
which would save some indentation as well. The Similar pattern can be
considered for 0004 and 0007 patches as well.
I kind of like it better this way. It preserves the functional style of
the original macro.
+static inline void
+XLogFromFileName(const char *fname, TimeLineID *tli, XLogSegNo
*logSegNo, int wal_segsz_bytes)
+{
+ uint32 log;
+ uint32 seg;
+ sscanf(fname, "%08X%08X%08X", tli, &log, &seg);
+ *logSegNo = (uint64) log * XLogSegmentsPerXLogId(wal_segsz_bytes) + seg;
+}
Can we have a blank line after variable declarations that we usually have?
done
0006 patch:
+static inline Datum
+fetch_att(const void *T, bool attbyval, int attlen)
+{
+ if (attbyval)
+ {
+#if SIZEOF_DATUM == 8
+ if (attlen == sizeof(Datum))
+ return *((const Datum *) T);
+ else
+#endif
Can we have a switch case like store_att_byval() instead of if-else,
code would look more symmetric, IMO.
doneFrom 75cc12a425a58340799376bc1e2dfdebfcb7ea0a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 23 May 2022 07:28:13 +0200
Subject: [PATCH v2] Convert macros to static inline functions (block.h)
Remove BlockIdIsValid(), which wasn't used and is unnecessary.
Remove BlockIdCopy(), which wasn't used and can be done by struct
assignment.
(BlockIdEquals() also isn't used, but seems reasonable to keep
around.)
Discussion:
https://www.postgresql.org/message-id/flat/5b558da8-99fb-0a99-83dd-f72f05388517%40enterprisedb.com
---
src/include/storage/block.h | 53 -
1 file changed, 23 insertions(+), 30 deletions(-)
diff --git a/src/include/storage/block.h b/src/include/storage/block.h
index d756e3fda5..c726142f96 100644
--- a/src/include/storage/block.h
+++ b/src/include/storage/block.h
@@ -59,7 +59,7 @@ typedef struct BlockIdData
typedef BlockIdData *BlockId; /* block identifier */
/*
- * support macros
+ * support functions
*
*/
@@ -67,49 +67,42 @@ typedef BlockIdData *BlockId; /* block identifier */
* BlockNumberIsValid
* True iff blockNumber is valid.
*/
-#define BlockNumberIsValid(blockNumber) \
- ((BlockNumber) (blockNumber) != InvalidBlockNumber)
-
-/*
- * BlockIdIsValid
- * True iff the block identifier is valid.
- */
-#define BlockIdIsValid(blockId) \
- PointerIsValid(blockId)
+static inline bool
+BlockNumberIsValid(BlockNumber blockNumber)
+{
+ return blockNumber != InvalidBlockNumber;
+}
/*
* BlockIdSet
* Sets a block identifier to the specified value.
*/
-#define BlockIdSet(blockId, blockNumber) \
-( \
- (blockId)->bi_hi = (blockNumber) >> 16, \
- (blockId)->bi_lo = (blockNumber) & 0x \
-)
-
-/*
- * BlockIdCopy
- * Copy a block identifier.
- */
-#define BlockIdCopy(toBlockId, fromBlockId) \
-( \
- (toBlockId)->bi_hi = (fromBlockId)->bi_hi, \
- (toBlockId)->bi_lo = (fromBlockId)->bi_lo \
-)
+static inline void
+BlockIdSet(BlockIdData *blockId, BlockNumber blockNumber)
+{
+ blockId->bi_hi = blockNumber >> 16;
+ blockId->bi_lo = blockNumber & 0x;
+}
/*
* BlockIdEquals
* Check for block number equality.
*/
-#define BlockIdEquals(blockId1, blockId2) \
- ((blockId1)->bi_hi == (blockId2)->bi_hi && \
-(blockId1)->bi_lo == (blockId2)->bi_lo)
+static inline bool
+BlockIdEquals(const BlockIdData *blockId1, const BlockIdData *blockId2)
+{
+ return (blockId1->bi_hi == blockId2->bi_hi &&
+ blockId1->bi_lo == blockId2->bi_lo);
+}
/*
* BlockIdGetBlockNumber
* Retrieve the block number from a block identifier.
*/
-#define BlockIdGetBlockNumber(blockId) \
- BlockNumber) (blockId)->bi_hi) << 16) | ((BlockNumber)
(blockId)->bi_lo))
+static inline BlockNumber
+BlockIdGetBlockNumber(const BlockIdData *blockId)
+{
+ return (((BlockNumber) (blockId)->bi_hi) << 16) | ((BlockNumber)
(blockId)->bi_lo);
+}
#endif /* BLOCK_H */
--
2.36.1
From 103f6b446cc2efa0b6a21bf181171cbad176d968 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 23 May 2022 07:28:13 +0200
Subject: [PATCH v2] Convert macros to static inline functions (bufpage.h)
rawpage.c needs some adjustments because it was previously playing
loose with the Page vs. PageHeader types, which is no longer possible
with the functions instead of macros.
Discussion:
https://www.postgresql.org/message-id/flat/5b558da8-99fb-0a99-83dd-f72f05388517%40enterprisedb.com
---
contrib/pageinspect/rawpage.c
Re: postgres_fdw has insufficient support for large object
On Mon, May 23, 2022 at 10:54 AM Tom Lane wrote:
>
> Dilip Kumar writes:
> > I don't think that the local pg_largeobject should maintain the
> > foreign server's data, instead that the export should fetch the data
> > from the remote's pg_largeobject table. Then I just checked inserting
> > into the foriegn from your test as shown below[1] and I noticed that
> > the insert is also importing the large object into the local
> > pg_largeobject instead of the remote server's pg_large object, which
> > clearly seems broken to me. Basically, the actual row is inserted on
> > the remote server and the large object w.r.t. the same row is imported
> > in local pg_largeobject.
>
> > insert into oid_table_ft
> > values(1,lo_import('/home/highgo/pictures/bird.jpg'));
>
> For this example to "work", lo_import() would have to somehow know
> that its result would get inserted into some foreign table and
> then go create the large object on that table's server instead
> of locally.
Yeah that makes sense. The lo_import() is just running as an
independent function to import the object into pg_largeobject and
return the Oid so definitely it has no business to know where that Oid
will be stored :)
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Re: postgres_fdw has insufficient support for large object
Dilip Kumar writes:
> I don't think that the local pg_largeobject should maintain the
> foreign server's data, instead that the export should fetch the data
> from the remote's pg_largeobject table. Then I just checked inserting
> into the foriegn from your test as shown below[1] and I noticed that
> the insert is also importing the large object into the local
> pg_largeobject instead of the remote server's pg_large object, which
> clearly seems broken to me. Basically, the actual row is inserted on
> the remote server and the large object w.r.t. the same row is imported
> in local pg_largeobject.
> insert into oid_table_ft
> values(1,lo_import('/home/highgo/pictures/bird.jpg'));
For this example to "work", lo_import() would have to somehow know
that its result would get inserted into some foreign table and
then go create the large object on that table's server instead
of locally.
This is unlikely to happen, for about ten different reasons that
you should have no trouble understanding if you stop to think
about it.
regards, tom lane
Re: postgres_fdw has insufficient support for large object
On Mon, May 23, 2022 at 7:16 AM Saladin wrote:
>
> PostgreSQL version:PostgreSQL 14.0 on x86_64-pc-linux-gnu, compiled by gcc
> (GCC) 4.8.5 20150623 (Red Hat 4.8.5-39), 64-bit
> Platform information:Linux version 3.10.0-1127.el7.x86_64
> (mockbu...@kbuilder.bsys.centos.org) (gcc version 4.8.5 20150623 (Red Hat
> 4.8.5-39) (GCC) ) #1 SMP Tue Mar 31 23:36:51 UTC 2020
>
> I created two tables for testing. One is remote table in database A and the
> other is foreign table in database B.
> Then i use INSERT statements with lo_import function to add data to remote
> table.
>
> The output i have got.
> The result is remote table,pg_largeobject in database
> A,pg_largeobject_metadata in database A have correct data.
> But,i don't find correct data in pg_largeobject and pg_largeobject_metadata
> in database B.
>
> My operation steps are as follows：
> Both database A and database B:
> create extension postgres_fdw;
> select * from pg_largeobject_metadata ;--check if exists any rows
> select * from pg_largeobject;
> database A:
> CREATE TABLE oid_table (id INT NOT NULL, oid_1 oid, oid_2 oid);
> insert into oid_table values
> (1,lo_import('/home/highgo/pictures/bird.jpg'),lo_import('/home/highgo/pictures/pig.jpg'));--Two
> ordinary files on the machine
> select * from oid_table;
> database B:
> CREATE server srv_postgres_cn_0 FOREIGN data wrapper postgres_fdw
> options(host '127.0.0.1', port '9000', dbname 'postgres');
> CREATE USER mapping FOR highgo server srv_postgres_cn_0 options(user
> 'highgo', password '123456');
> CREATE FOREIGN TABLE oid_table_ft (id INT NOT NULL, oid_1 oid, oid_2
> oid) server srv_postgres_cn_0 options(schema_name 'public', table_name
> 'oid_table');
> select * from oid_table_ft;
> select lo_export(oid_1,'/usr/local/pgsql/out.jpg') from oid_table_ft where
> id=1;--the result is "ERROR: large object xxx does not exist"
>
> comments :
> my default databse is "postgres" and default user is "highgo" and I don't
> think these will have an impact on this problem.
>
> The output i expected:
> pg_largeobject_metadata and pg_largeobject in both database A and database
> B should have rows.Shouldn't only in database A.So, i can use large object
> functions
> to operate large_objectin remote table or foreign table.
I don't think that the local pg_largeobject should maintain the
foreign server's data, instead that the export should fetch the data
from the remote's pg_largeobject table. Then I just checked inserting
into the foriegn from your test as shown below[1] and I noticed that
the insert is also importing the large object into the local
pg_largeobject instead of the remote server's pg_large object, which
clearly seems broken to me. Basically, the actual row is inserted on
the remote server and the large object w.r.t. the same row is imported
in local pg_largeobject.
insert into oid_table_ft values(1,lo_import('/home/highgo/pictures/bird.jpg'));
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Re: Enforce "max_wal_size/ min_wal_size must be at least twice wal_segment_size" limit while setting GUCs
At Sat, 21 May 2022 19:08:06 +0530, Bharath Rupireddy wrote in > How about we add GUC check hooks for both max_wal_size and > min_wal_size where we can either emit ERROR or WARNING if values are > not "at least twice as wal_segment_size"? It should be ERROR. As you say, it should have been changed when the unit of them is changed to MB and wal_segment_size became variable. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Skipping schema changes in publication
On Sat, May 21, 2022 at 11:06 AM vignesh C wrote:
>
> On Fri, May 20, 2022 at 11:23 AM Peter Smith wrote:
> >
> > Below are my review comments for v6-0002.
> >
> > ==
> >
> > 1. Commit message.
> > The psql \d family of commands to display excluded tables.
> >
> > SUGGESTION
> > The psql \d family of commands can now display excluded tables.
>
> Modified
>
> > ~~~
> >
> > 2. doc/src/sgml/ref/alter_publication.sgml
> >
> > @@ -22,6 +22,7 @@ PostgreSQL documentation
> >
> >
> > ALTER PUBLICATION name
> > ADD publication_object [,
> > ...]
> > +ALTER PUBLICATION name
> > ADD ALL TABLES [ EXCEPT [ TABLE ] exception_object [, ... ] ]
> >
> > The "exception_object" font is wrong. Should look the same as
> > "publication_object"
>
> Modified
>
> > ~~~
> >
> > 3. doc/src/sgml/ref/alter_publication.sgml - Examples
> >
> > @@ -214,6 +220,14 @@ ALTER PUBLICATION sales_publication ADD ALL
> > TABLES IN SCHEMA marketing, sales;
> >
> >
> >
> > +
> > + Alter publication production_publication to
> > publish
> > + all tables except users and
> > + departments tables:
> > +
> > +ALTER PUBLICATION production_publication ADD ALL TABLES EXCEPT TABLE
> > users, departments;
> > +
> >
> > Consider using "EXCEPT" instead of "EXCEPT TABLE" because that will
> > show TABLE keyword is optional.
>
> Modified
>
> > ~~~
> >
> > 4. doc/src/sgml/ref/create_publication.sgml
> >
> > An SGML tag error caused building the docs to fail. My fix was
> > previously reported [1].
>
> Modified
>
> > ~~~
> >
> > 5. doc/src/sgml/ref/create_publication.sgml
> >
> > @@ -22,7 +22,7 @@ PostgreSQL documentation
> >
> >
> > CREATE PUBLICATION name
> > -[ FOR ALL TABLES
> > +[ FOR ALL TABLES [ EXCEPT [ TABLE ] exception_object [, ... ] ]
> >
> > The "exception_object" font is wrong. Should look the same as
> > "publication_object"
>
> Modified
>
> > ~~~
> >
> > 6. doc/src/sgml/ref/create_publication.sgml - Examples
> >
> > @@ -351,6 +366,15 @@ CREATE PUBLICATION production_publication FOR
> > TABLE users, departments, ALL TABL
> > CREATE PUBLICATION sales_publication FOR ALL TABLES IN SCHEMA marketing,
> > sales;
> >
> >
> > +
> > + Create a publication that publishes all changes in all the tables
> > except for
> > + the changes of users and
> > + departments table:
> > +
> > +CREATE PUBLICATION mypublication FOR ALL TABLE EXCEPT TABLE users,
> > departments;
> > +
> > +
> > +
> >
> > 6a.
> > Typo: "FOR ALL TABLE" -> "FOR ALL TABLES"
>
> Modified
>
> > 6b.
> > Consider using "EXCEPT" instead of "EXCEPT TABLE" because that will
> > show TABLE keyword is optional.
>
> Modified
>
> > ~~~
> >
> > 7. src/backend/catalog/pg_publication.c - GetTopMostAncestorInPublication
> >
> > @@ -316,18 +316,25 @@ GetTopMostAncestorInPublication(Oid puboid, List
> > *ancestors, int *ancestor_level
> > }
> > else
> > {
> > - aschemaPubids = GetSchemaPublications(get_rel_namespace(ancestor));
> > - if (list_member_oid(aschemaPubids, puboid))
> > + List*aschemapubids = NIL;
> > + List*aexceptpubids = NIL;
> > +
> > + aschemapubids = GetSchemaPublications(get_rel_namespace(ancestor));
> > + aexceptpubids = GetRelationPublications(ancestor, true);
> > + if (list_member_oid(aschemapubids, puboid) ||
> > + (puballtables && !list_member_oid(aexceptpubids, puboid)))
> > {
> >
> > You could re-write this as multiple conditions instead of one. That
> > could avoid always assigning the 'aexceptpubids', so it might be a
> > more efficient way to write this logic.
>
> Modified
>
> > ~~~
> >
> > 8. src/backend/catalog/pg_publication.c - CheckPublicationDefValues
> >
> > +/*
> > + * Check if the publication has default values
> > + *
> > + * Check the following:
> > + * Publication is having default options
> > + * Publication is not associated with relations
> > + * Publication is not associated with schemas
> > + * Publication is not set with "FOR ALL TABLES"
> > + */
> > +static bool
> > +CheckPublicationDefValues(HeapTuple tup)
> >
> > 8a.
> > Remove the tab. Replace with spaces.
>
> Modified
>
> > 8b.
> > It might be better if this comment order is the same as the logic order.
> > e.g.
> >
> > * Check the following:
> > * Publication is not set with "FOR ALL TABLES"
> > * Publication is having default options
> > * Publication is not associated with schemas
> > * Publication is not associated with relations
>
> Modified
>
> > ~~~
> >
> > 9. src/backend/catalog/pg_publication.c - AlterPublicationSetAllTables
> >
> > +/*
> > + * Reset the publication.
> > + *
> > + * Reset the publication options, publication relations and
> > publication schemas.
> > + */
> > +static void
> > +AlterPublicationSetAllTables(Relation rel, HeapTuple tup)
> >
> > The function comment and the function name do not seem to match here;
> > something looks like a cut/paste error ??
>
> Modified
>
> > ~~~
> >
> > 10. src/backend/catalog/pg_publication.c - AlterPublicationSetAllTables
> >
> > + /* set all tables option */
> > + values
Re: Enforce "max_wal_size/ min_wal_size must be at least twice wal_segment_size" limit while setting GUCs
On Sat, May 21, 2022 at 11:26 PM rajesh singarapu wrote: > > On Sat, May 21, 2022 at 7:08 PM Bharath Rupireddy > wrote: > > > > Hi, > > > > Currently postgres allows setting any value for max_wal_size or > > min_wal_size, not enforcing "at least twice as wal_segment_size" limit > > [1]. This isn't a problem if the server continues to run, however, the > > server can't come up after a crash or restart or maintenance or > > upgrade (goes to a continuous restart loop with FATAL errors [1]). > > > > How about we add GUC check hooks for both max_wal_size and > > min_wal_size where we can either emit ERROR or WARNING if values are > > not "at least twice as wal_segment_size"? > > > > Thoughts? > > > > [1] > > FATAL: "max_wal_size" must be at least twice "wal_segment_size" > > FATAL: "min_wal_size" must be at least twice "wal_segment_size" > Hi Bharath, > > Could you explain why min wal size must be at least twice but not > equal to wal_segment_size ? It is because postgres always needs/keeps at least one WAL file and the usage of max_wal_size/min_wal_size is in terms of number of WAL segments/WAL files. It doesn't make sense to set max_wal_size/min_wal_size to, say, 20MB (where wal_segment_size = 16MB) and expect the server to honor it and do something. Hence the 'at least twice' requirement. Regards, Bharath Rupireddy.
Re: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns
At Sat, 21 May 2022 15:35:58 +0530, Amit Kapila wrote in > I think if we don't have any better ideas then we should go with > either this or one of the other proposals in this thread. The other > idea that occurred to me is whether we can somehow update the snapshot > we have serialized on disk about this information. On each > running_xact record when we serialize the snapshot, we also try to > purge the committed xacts (via SnapBuildPurgeCommittedTxn). So, during > that we can check if there are committed xacts to be purged and if we > have previously serialized the snapshot for the prior running xact > record, if so, we can update it with the list of xacts that have > catalog changes. If this is feasible then I think we need to somehow > remember the point where we last serialized the snapshot (maybe by > using builder->last_serialized_snapshot). Even, if this is feasible we > may not be able to do this in back-branches because of the disk-format > change required for this. > > Thoughts? I didn't look it closer, but it seems to work. I'm not sure how much spurious invalidations at replication start impacts on performance, but it is promising if the impact is significant. That being said I'm a bit negative for doing that in post-beta1 stage. I thought for a moment that RUNNING_XACT might be able to contain invalidation information but it seems too complex to happen with such a frequency.. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Zstandard support for toast compression
On Thu, May 19, 2022 at 04:12:01PM -0400, Robert Haas wrote: > On Thu, May 19, 2022 at 4:20 AM Michael Paquier wrote: >> Btw, shouldn't we have something a bit more, err, extensible for the >> design of an extensible varlena header? If we keep it down to some >> bitwise information, we'd be fine for a long time but it would be >> annoying to review again an extended design if we need to extend it >> with more data. > > What do you have in mind? A per-varlena checksum was one thing that came into my mind. -- Michael signature.asc Description: PGP signature
Re: Handle infinite recursion in logical replication setup
On Fri, May 20, 2022 at 3:08 PM vignesh C wrote:
>
> On Wed, May 18, 2022 at 10:29 AM Amit Kapila wrote:
> >
> >
> > Few comments on v13-0001
> > ==
> > 1.
> > + *
> > + * FIXME: LOGICALREP_PROTO_LOCALONLY_VERSION_NUM needs to be bumped to 4 in
> > + * PG16.
> > ...
> > @@ -477,6 +489,12 @@ pgoutput_startup(LogicalDecodingContext *ctx,
> > OutputPluginOptions *opt,
> > else
> > ctx->twophase_opt_given = true;
> >
> > + if (data->local_only && data->protocol_version <
> > LOGICALREP_PROTO_LOCALONLY_VERSION_NUM)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("requested proto_version=%d does not support local_only, need
> > %d or higher",
> > + data->protocol_version, LOGICALREP_PROTO_LOCALONLY_VERSION_NUM)));
> >
> > What is the need to change the protocol version for this parameter? As
> > per my understanding, we only need a different protocol version when
> > we are sending some new message or some additional information in an
> > existing message as we have done for the streaming/two_phase options
> > which doesn't seem to be the case here.
>
> Modified
>
It seems you forgot to remove the comments after removing the code
corresponding to the above. See below.
+ *
+ * LOGICALREP_PROTO_LOCALONLY_VERSION_NUM is the minimum protocol version with
+ * support for sending only locally originated data from the publisher.
+ * Introduced in PG16.
+ *
+ * FIXME: LOGICALREP_PROTO_LOCALONLY_VERSION_NUM needs to be bumped to 4 in
+ * PG16.
*/
Few other comments on 0001
1.
+ * Return true if data has originated remotely when only_local option is
+ * enabled, false otherwise.
Can we slightly change the comment to:"Return true if the data source
(origin) is remote and user has requested only local data, false
otherwise."
2.
+SELECT pg_replication_origin_session_reset();
+SELECT pg_drop_replication_slot('regression_slot_only_local');
+SELECT pg_replication_origin_drop('regress_test_decoding:
regression_slot_only_local');
\ No newline at end of file
At the end of the file, there should be a new line.
3.
+ subonlylocal bool
+
+
+ If true, subscription will request the publisher to send locally
+ originated changes at the publisher node, or send any publisher node
+ changes regardless of their origin
The case where this option is not set is not clear from the
description. Can we change the description to: "If true, the
subscription will request that the publisher send locally originated
changes. False indicates that the publisher sends any changes
regardless of their origin."
4. This new option 'subonlylocal' is placed before 'substream' in docs
(catalogs.sgml) and after it in structures in pg_subscription.h. I
suggest adding it after 'subdisableonerr' in docs and
pg_subscription.h. Also, adjust other places in subscriber-side code
to place it consistently.
5.
@@ -4516,6 +4524,8 @@ getSubscriptions(Archive *fout)
pg_strdup(PQgetvalue(res, i, i_subtwophasestate));
subinfo[i].subdisableonerr =
pg_strdup(PQgetvalue(res, i, i_subdisableonerr));
+ subinfo[i].subonlylocal =
+ pg_strdup(PQgetvalue(res, i, i_subonlylocal));
/* Decide whether we want to dump it */
selectDumpableObject(&(subinfo[i].dobj), fout);
@@ -4589,6 +4599,9 @@ dumpSubscription(Archive *fout, const
SubscriptionInfo *subinfo)
if (strcmp(subinfo->subdisableonerr, "t") == 0)
appendPQExpBufferStr(query, ", disable_on_error = true");
+ if (strcmp(subinfo->subonlylocal, "t") == 0)
+ appendPQExpBufferStr(query, ", only_local = true");
+
if (strcmp(subinfo->subsynccommit, "off") != 0)
appendPQExpBuffer(query, ", synchronous_commit = %s",
fmtId(subinfo->subsynccommit));
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 1d21c2906f..ddb855fd16 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -661,6 +661,7 @@ typedef struct _SubscriptionInfo
char*subdisableonerr;
char*subsynccommit;
char*subpublications;
+ char*subonlylocal;
} SubscriptionInfo;
To keep this part of the code consistent, I think it is better to
place 'subonlylocal' after 'subdisableonerr' in SubscriptionInfo.
--
With Regards,
Amit Kapila.
Re: PG15 beta1 fix pg_database view document
On Mon, May 23, 2022 at 02:00:18AM +, Shinoda, Noriyoshi (PN Japan FSIP) wrote: > Thanks to all the developers. The attached patch updates the manual > for the pg_database catalog. > The current pg_database view definition is missing the > datlocprovider column. The attached patch adds this column info. > https://www.postgresql.org/docs/15/catalog-pg-database.html Indeed. I have checked the rest of the catalog headers for any inconsistencies with the docs and what you have noticed here is the only one. + datlocprovider char + + + Provider of the collation for this database: c = libc, i = icu + ICU needs to be upper-case if you are referring to the library name. Here, my guess is that you are referring to the value that can be passed down to the command? You could use lower-case terms like on the CREATE COLLATION page, but I would put these within a markup. Anyway, this formulation is incorrect. -- Michael signature.asc Description: PGP signature
PG15 beta1 fix pg_database view document
Hi hackers, Thanks to all the developers. The attached patch updates the manual for the pg_database catalog. The current pg_database view definition is missing the datlocprovider column. The attached patch adds this column info. https://www.postgresql.org/docs/15/catalog-pg-database.html Regards, Noriyoshi Shinoda pg_database_doc_v1.diff Description: pg_database_doc_v1.diff
postgres_fdw has insufficient support for large object
PostgreSQL version:PostgreSQL 14.0 on x86_64-pc-linux-gnu, compiled by gcc
(GCC) 4.8.5 20150623 (Red Hat 4.8.5-39), 64-bit
Platform information:Linux version 3.10.0-1127.el7.x86_64
(mockbu...@kbuilder.bsys.centos.org) (gcc version 4.8.5 20150623 (Red Hat
4.8.5-39) (GCC) ) #1 SMP Tue Mar 31 23:36:51 UTC 2020
I created two tables for testing. One is remote table in database A and the
other is foreign table in database B.
Then i use INSERT statements with lo_import function to add data to remote
table.
The output i have got.
The result is remote table,pg_largeobject in database
A,pg_largeobject_metadata in database A have correct data.
But,i don't find correct data in pg_largeobject and pg_largeobject_metadata
in database B.
My operation steps are as follows??
Both database A and database B:
create extension postgres_fdw;
select * from pg_largeobject_metadata ;--check if exists any rows
select * from pg_largeobject;
database A:
CREATE TABLE oid_table (id INT NOT
NULL, oid_1 oid, oid_2 oid);
insert into oid_table values
(1,lo_import('/home/highgo/pictures/bird.jpg'),lo_import('/home/highgo/pictures/pig.jpg'));--Two
ordinary files on the machine
select * from oid_table;
database B:
CREATE server srv_postgres_cn_0
FOREIGN data wrapper postgres_fdw
options(host '127.0.0.1', port '9000', dbname 'postgres');
CREATE USER mapping FOR highgo
server srv_postgres_cn_0 options(user
'highgo', password '123456');
CREATE FOREIGN TABLE oid_table_ft
(id INT NOT NULL, oid_1 oid, oid_2
oid) server srv_postgres_cn_0 options(schema_name 'public', table_name
'oid_table');
select * from oid_table_ft;
select lo_export(oid_1,'/usr/local/pgsql/out.jpg') from oid_table_ft where
id=1;--the result is "ERROR: large object xxx does not exist"
comments :
my default databse is "postgres" and default user is "highgo" and I don't
think these will have an impact on this problem.
The output i expected:
pg_largeobject_metadata and pg_largeobject in both database A and database
B should have rows.Shouldn't only in database A.So, i can use large object
functions
to operate large_objectin remote table or foreign table.
Please forgive me, English is not my mother tongue. If you have any doubts
about my description, please contact me, and I will reply to you at the
first time. Thank you sincerely and look forward to your reply.
Re: Add --{no-,}bypassrls flags to createuser
At Sun, 22 May 2022 09:55:37 +0200, Przemysław Sztoch wrote in > David G. Johnston wrote on 5/19/2022 3:46 AM: > > As an aside, I'd rather overcome this particular objection by having > > the CREATE object command all accept an optional "COMMENT IS" clause. > > > I believe that it is not worth dividing it into a separate program. > > The same --comment argument is needed for the createdb command. David didn't say that it should be another "program", but said it should be another "patch/development", because how we implement the --comment feature is apparently controversial. It doesn't seem to be explicity mentioned that "createuser is mere a shell-substitute for the SQL CREATE ROLE", but I feel the same with Shinya that it is. We could directly invoke "COMMENT ON" from createuser command, but I think it is not the way to go in that light. We can either add COMMENT clause only to "CREATE ROLE" , or "COMMENT IS" clause to all (or most of) "CREATE object" commands, or something others. (Perhaps "COMMETN IS" requires "ALTER object" handle comments, and I'm not sure how we think about the difference of it from "comment on" command.) We might return to "comment on" in the end.. Anyway, after fixing that issue we will modify the createrole command so that it uses the new SQL feature. I find no hard obstacles in reaching there in the 16 cycle. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Is RecoveryConflictInterrupt() entirely safe in a signal handler?
Hi,
It'd be cool to commit and backpatch this - I'd like to re-enable the conflict
tests in the backbranches, and I don't think we want to do so with this issue
in place.
On 2022-05-10 16:39:11 +1200, Thomas Munro wrote:
> On Tue, Apr 12, 2022 at 10:50 AM Andres Freund wrote:
> > On 2022-04-12 10:33:28 +1200, Thomas Munro wrote:
> > > Instead of bothering to create N different XXXPending variables for
> > > the different conflict "reasons", I used an array. Other than that,
> > > it's much like existing examples.
> >
> > It kind of bothers me that each pending conflict has its own external
> > function
> > call. It doesn't actually cost anything, because it's quite unlikely that
> > there's more than one pending conflict. Besides aesthetically displeasing,
> > it also leads to an unnecessarily large amount of code needed, because the
> > calls to RecoveryConflictInterrupt() can't be merged...
>
> Ok, in this version there's two levels of flag:
> RecoveryConflictPending, so we do nothing if that's not set, and then
> the loop over RecoveryConflictPendingReasons is moved into
> ProcessRecoveryConflictInterrupts(). Better?
I think so.
I don't particularly like the Handle/ProcessRecoveryConflictInterrupt() split,
naming-wise. I don't think Handle vs Process indicates something meaningful?
Maybe s/Interrupt/Signal/ for the signal handler one could help?
It *might* look a tad cleaner to have the loop in a separate function from the
existing code. I.e. a +ProcessRecoveryConflictInterrupts() that calls
ProcessRecoveryConflictInterrupts().
> > What might actually make more sense is to just have a bitmask or something?
>
> Yeah, in fact I'm exploring something like that in later bigger
> redesign work[1] that gets rid of signal handlers. Here I'm looking
> for something simple and potentially back-patchable and I don't want
> to have to think about async signal safety of bit-level manipulations.
Makes sense.
> /*
> @@ -3146,6 +3192,9 @@ ProcessInterrupts(void)
> return;
> InterruptPending = false;
>
> + if (RecoveryConflictPending)
> + ProcessRecoveryConflictInterrupts();
> +
> if (ProcDiePending)
> {
> ProcDiePending = false;
Should the ProcessRecoveryConflictInterrupts() call really be before the
ProcDiePending check?
Greetings,
Andres Freund
Re: docs: mention "pg_read_all_stats" in "track_activities" description
On Sun, May 22, 2022 at 01:26:08PM -0700, Nathan Bossart wrote: > Yeah, this crossed my mind. I thought that "superusers, roles with > privileges of the pg_read_all_stats_role, roles with privileges of the user > owning the session being reported on, and the user owning the session being > reported on" might be too long-winded and redundant. But I see your point > that it might be a bit confusing. Perhaps it could be trimmed down to > something like this: > > ... superusers, roles with privileges of the pg_read_all_stats role, > and roles with privileges of the user owning the session being reported > on (including the session owner). Yeah, that sounds better to me. monitoring.sgml has a different way of wording what looks like the same thing for pg_stat_xact_*_tables: "Ordinary users can only see all the information about their own sessions (sessions belonging to a role that they are a member of)". So you could say instead something like: this information is only visible to superusers, roles with privileges of the pg_read_all_stats role, and the user owning the sessionS being reported on (including sessions belonging to a role that they are a member of). -- Michael signature.asc Description: PGP signature
ccache, MSVC, and meson
forking: <20220307191054.n5enrlf6kdn7z...@alap3.anarazel.de> An update. ccache 4.6.1 was released which allows compiling postgres I submitted a request to update the package in chocolatey. But with the existing build system, it's no faster anyway, I guess due to poor use of parallelism. https://cirrus-ci.com/task/5972008205811712 Currently, meson doesn't (automatically) use ccache with MSVC - see mesonbuild/environment.py. And CC=ccache gives an error - I suppose it should not try to pop ccache off the compiler list if the list has only one element. |[21:44:49.791] File "C:\python\lib\site-packages\mesonbuild\compilers\detect.py", line 375, in _detect_c_or_cpp_compiler |[21:44:49.791] compiler_name = os.path.basename(compiler[0]) |[21:44:49.791] IndexError: list index out of range |... |[21:44:49.791] meson.build:1:0: ERROR: Unhandled python exception |[21:44:49.791] |[21:44:49.791] This is a Meson bug and should be reported! But it can be convinced to use ccache by renaming the executable to "pgccache". Which builds in 46sec: https://cirrus-ci.com/task/4862234995195904 This requires ccache 4.6, released in Feburary and already in choco. Note that ccache supports neither /Zi debugging nor precompiled headers. I'm not sure, but -Dc_args=/Z7 may do what's wanted here.
Re: 15beta1 test failure on mips in isolation/expected/stats
Hi, On 2022-05-20 01:25:10 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2022-05-20 00:22:14 -0400, Tom Lane wrote: > >> There's some fallout in the expected-file, of course, but this > >> does seem to fix it (20 consecutive successful runs now at > >> 100/2). Don't see why though ... > > > I think what might be happening is that the transactional stats updates get > > reported by s2 *before* the non-transactional stats updates come in from > > s1. I.e. the pgstat_report_stat() at the end of s2_commit_prepared_a does a > > report, because the machine is slow enough for it to be "time to reports > > stats > > again". Then s1 reports its non-transactional stats. > > Sounds plausible. And I left the test loop running, and it's now past > 100 consecutive successes, so I think this change definitely "fixes" it. FWIW, the problem can be reliably reproduced by sticking a pgstat_force_next_flush() into pgstat_twophase_postcommit(). This is the only failure when doing so. > > It looks like our stats maintenance around truncation isn't quite > > "concurrency > > safe". That code hasn't meaningfully changed, but it'd not be surprising if > > it's not 100% precise... > > Yeah. Probably not something to try to improve post-beta, especially > since it's not completely clear how transactional and non-transactional > cases *should* interact. Yea. It's also not normally particularly crucial to be accurate down to that degree. > Maybe non-transactional updates should be > pushed immediately? But I'm not sure if that's fully correct, and > it definitely sounds expensive. I think that'd be far too expensive - the majority of stats are non-transactional... I think what we could do is to model truncates as subtracting the number of live/dead rows the truncating backend knows about, rather than setting them to 0. But that of course could incur other inaccuracies. > I'd be good with tweaking this test case as you suggest, and maybe > revisiting the topic later. Pushed the change of the test. Christoph, just to make sure, can you confirm that this fixes the test instability for you? > Kyotaro-san worried about whether any other places in stats.spec > have the same issue. I've not seen any evidence of that in my > tests, but perhaps some other machine with different timing > could find it. I tried to find some by putting in forced flushes in a bunch of places before, and now some more, without finding further cases. Greetings, Andres Freund
Re: docs: mention "pg_read_all_stats" in "track_activities" description
On Sun, May 22, 2022 at 09:59:47AM +0900, Michael Paquier wrote: > +visible to superusers, roles with privileges of the > +pg_read_all_stats role, and roles with privileges of > +the user owning the session being reported on, so it should not > +represent a security risk. Only superusers and users with the > +appropriate SET privilege can change this setting. > > Regarding the fact that a user can see its own information, the last > part of the description would be right, still a bit confusing perhaps > when it comes to one's own information? Yeah, this crossed my mind. I thought that "superusers, roles with privileges of the pg_read_all_stats_role, roles with privileges of the user owning the session being reported on, and the user owning the session being reported on" might be too long-winded and redundant. But I see your point that it might be a bit confusing. Perhaps it could be trimmed down to something like this: ... superusers, roles with privileges of the pg_read_all_stats role, and roles with privileges of the user owning the session being reported on (including the session owner). -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: PG15 beta1 sort performance regression due to Generation context change
Hi David, >Over the past few days I've been gathering some benchmark results >together to show the sort performance improvements in PG15 [1]. >One of the test cases I did was to demonstrate Heikki's change to use >a k-way merge (65014000b). >The test I did to try this out was along the lines of: >set max_parallel_workers_per_gather = 0; >create table t (a bigint not null, b bigint not null, c bigint not >null, d bigint not null, e bigint not null, f bigint not null); >insert into t select x,x,x,x,x,x from generate_Series(1,140247142) x; -- 10GB! >vacuum freeze t; >The query I ran was: >select * from t order by a offset 140247142; I redid this test here: Windows 10 64 bits msvc 2019 64 bits RAM 8GB SSD 256 GB HEAD (default configuration) Time: 229396,551 ms (03:49,397) PATCHED: Time: 220887,346 ms (03:40,887) >I tested various sizes of work_mem starting at 4MB and doubled that >all the way to 16GB. For many of the smaller values of work_mem the >performance is vastly improved by Heikki's change, however for >work_mem = 64MB I detected quite a large slowdown. PG14 took 20.9 >seconds and PG15 beta 1 took 29 seconds! >I've been trying to get to the bottom of this today and finally have >discovered this is due to the tuple size allocations in the sort being >exactly 64 bytes. Prior to 40af10b57 (Use Generation memory contexts >to store tuples in sorts) the tuple for the sort would be stored in an >aset context. After 40af10b57 we'll use a generation context. The >idea with that change is that the generation context does no >power-of-2 round ups for allocations, so we save memory in most cases. >However, due to this particular test having a tuple size of 64-bytes, >there was no power-of-2 wastage with aset. >The problem is that generation chunks have a larger chunk header than >aset do due to having to store the block pointer that the chunk >belongs to so that GenerationFree() can increment the nfree chunks in >the block. aset.c does not require this as freed chunks just go onto a >freelist that's global to the entire context. >Basically, for my test query, the slowdown is because instead of being >able to store 620702 tuples per tape over 226 tapes with an aset >context, we can now only store 576845 tuples per tape resulting in >requiring 244 tapes when using the generation context. >If I had added column "g" to make the tuple size 72 bytes causing >aset's code to round allocations up to 128 bytes and generation.c to >maintain the 72 bytes then the sort would have stored 385805 tuples >over 364 batches for aset and 538761 tuples over 261 batches using the >generation context. That would have been a huge win. >So it basically looks like I discovered a very bad case that causes a >significant slowdown. Yet other cases that are not an exact power of >2 stand to gain significantly from this change. >One thing 40af10b57 does is stops those terrible performance jumps >when the tuple size crosses a power-of-2 boundary. The performance >should be more aligned to the size of the data being sorted now... >Unfortunately, that seems to mean regressions for large sorts with >power-of-2 sized tuples. It seems to me that the solution would be to use aset allocations when the size of the tuples is power-of-2? if (state->sortopt & TUPLESORT_ALLOWBOUNDED || (state->memtupsize & (state->memtupsize - 1)) == 0) state->tuplecontext = AllocSetContextCreate(state->sortcontext, "Caller tuples", ALLOCSET_DEFAULT_SIZES); else state->tuplecontext = GenerationContextCreate(state->sortcontext, "Caller tuples", ALLOCSET_DEFAULT_SIZES); I took a look and tried some improvements to see if I had a better result. Would you mind taking a look and testing? regards, Ranier Vilela CREATE TABLE t_test (x numeric); INSERT INTO t_test SELECT random() FROM generate_series(1, 500); ANALYZE; SHOW work_mem; HEAD: postgres=# explain analyze SELECT * FROM t_test ORDER BY x; QUERY PLAN Gather Merge (cost=397084.73..883229.71 rows=416 width=11) (actual time=1326.281..2718.040 rows=500 loops=1) Workers Planned: 2 Workers Launched: 2 -> Sort (cost=396084.71..401293.04 rows=208 width=11) (actual time=1287.168..1520.520 rows=167 loops=3) Sort Key: x Sort Method: external merge Disk: 24880kB Worker 0: Sort Method: external merge Disk: 24776kB Worker 1: Sort Method: external merge Disk: 23960kB -> Parallel Seq Scan on t_test (cost=0.00..47861.33 rows=208 width=11) (actual time=0.241..135.730 rows=167 loops=3) Planning Time: 0.054 ms Execution Time: 2837.789 ms (11 rows) PATCHED: postgres=# explain analyze SELECT * FROM t_test ORDER BY x; QUERY PLAN ---
Re: Unsubscribing from this mailing list.
Greetings (everyone except Israa),
* Israa Odeh (israa.k.o...@gmail.com) wrote:
>Please I need to cancel my subscription to this mailing list, could you
>delete me from it, or tell me how to unsubscribe.
To hopefully forstall general replies and such to this- they've already
been unsubscribed (and notified of that).
I'll look at updating our unsubscribe-detection logic to pick up on
variations like this ("Unsubscribing" isn't detected, "Unsubscribe" is)
to avoid these making it to the list in the future.
Thanks,
Stephen
signature.asc
Description: PGP signature
Re: Add --{no-,}bypassrls flags to createuser
David G. Johnston wrote on 5/19/2022 3:46 AM: I think that this feature is at least worth considering - but absent an existing command that does this I would agree that doing so constitutes a separate feature. As an aside, I'd rather overcome this particular objection by having the CREATE object command all accept an optional "COMMENT IS" clause. David J. The createuser command is typically used by IT personnel unfamiliar with SQL and unfamiliar with psql. They often use this command because software installation procedures require it. Lack of comment then causes more mess in the configuration of larger servers. I still think it's worth adding the --comment argument to execute the next SQL statement by createuser. This will simplify the setup scripts and installation instructions for the final software. I believe that it is not worth dividing it into a separate program. The same --comment argument is needed for the createdb command. -- Przemysław Sztoch | Mobile +48 509 99 00 66
Re: Patch: Don't set LoadedSSL unless secure_initialize succeeds
> On 22 May 2022, at 08:41, Gurjeet Singh wrote: > The initialization in PostmasterMain() blindly turns on LoadedSSL, > irrespective of the outcome of secure_initialize(). This call is invoked with isServerStart set to true so any error in secure_initialize should error out with ereport FATAL (in be_tls_init()). That could be explained in a comment though, which is currently isn't. Did you manage to get LoadedSSL set to true without SSL having been properly initialized? -- Daniel Gustafsson https://vmware.com/
