Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Wed, Oct 27, 2021 at 3:18 AM Bossart, Nathan wrote: > > On 10/26/21, 2:04 PM, "Jeff Davis" wrote: > > Should we just add a builtin function pg_checkpoint(), and deprecate > > the syntax? > > That seems reasonable to me. IMHO, moving away from SQL command "CHECKPOINT" to function "pg_checkpoint()" isn't nice as the SQL command has been there for a long time and all the applications or services that were/are being built around the postgres ecosystem would have to adapt someday to the new function (if at all we deprecate the command and onboard the function). This isn't good at all given the CHECKPOINT is one of the mostly used commands in the apps or services layer. Moreover, if we go with the function pg_checkpoint(), we might see patches coming in for pg_vacuum(), pg_reindex(), pg_cluster() and so on. I suggest having a predefined role (pg_maintenance or pg_checkpoint(although I'm not sure convinced to have a separate role just for checkpoint) or some other) and let superuser and the users with this new predefined role do checkpoint. Regards, Bharath Rupireddy.
Opclass parameters of indexes lost after REINDEX CONCURRENTLY
Hi all,
While reviewing the code for opclass parameters with indexes, I have
noticed that opclass parameters are lost after a concurrent reindex.
As we use a IndexInfo to hold the information of the new index when
creating a copy of the old one, it is just a matter of making sure
that ii_OpclassOptions is filled appropriately, but that was missed by
911e702.
Attached is a patch to fix the issue. After a concurrent reindex, we
would not finish with a corrupted index, just with one rebuilt with
default opclass parameter values.
Any objections or comments?
--
Michael
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 26bfa74ce7..460f84d4e8 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1365,6 +1365,15 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
newInfo->ii_IndexAttrNumbers[i] = oldInfo->ii_IndexAttrNumbers[i];
}
+ /* Extract opclass parameters for each attribute, if any */
+ if (oldInfo->ii_OpclassOptions)
+ {
+ newInfo->ii_OpclassOptions = palloc0(sizeof(Datum) *
+ newInfo->ii_NumIndexAttrs);
+ for (int i = 0; i < oldInfo->ii_NumIndexAttrs; i++)
+ newInfo->ii_OpclassOptions[i] = oldInfo->ii_OpclassOptions[i];
+ }
+
/*
* Now create the new index.
*
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 4750eac359..4702333fd6 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2176,6 +2176,24 @@ SELECT indexrelid::regclass, indisreplident FROM pg_index
(1 row)
DROP TABLE concur_replident;
+-- Check that opclass parameters are preserved
+CREATE TABLE concur_appclass_tab(i tsvector, j tsvector);
+CREATE INDEX concur_appclass_ind on concur_appclass_tab
+ USING gist (i tsvector_ops (siglen='1000'), j tsvector_ops (siglen='500'));
+CREATE INDEX concur_appclass_ind_2 on concur_appclass_tab
+ USING gist (i tsvector_ops, j tsvector_ops (siglen='300'));
+REINDEX TABLE CONCURRENTLY concur_appclass_tab;
+\d concur_appclass_tab
+ Table "public.concur_appclass_tab"
+ Column | Type | Collation | Nullable | Default
++--+---+--+-
+ i | tsvector | | |
+ j | tsvector | | |
+Indexes:
+"concur_appclass_ind" gist (i tsvector_ops (siglen='1000'), j tsvector_ops (siglen='500'))
+"concur_appclass_ind_2" gist (i, j tsvector_ops (siglen='300'))
+
+DROP TABLE concur_appclass_tab;
-- Partitions
-- Create some partitioned tables
CREATE TABLE concur_reindex_part (c1 int, c2 int) PARTITION BY RANGE (c1);
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index 22209b0691..c26f6ff70b 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -888,6 +888,15 @@ REINDEX TABLE CONCURRENTLY concur_replident;
SELECT indexrelid::regclass, indisreplident FROM pg_index
WHERE indrelid = 'concur_replident'::regclass;
DROP TABLE concur_replident;
+-- Check that opclass parameters are preserved
+CREATE TABLE concur_appclass_tab(i tsvector, j tsvector);
+CREATE INDEX concur_appclass_ind on concur_appclass_tab
+ USING gist (i tsvector_ops (siglen='1000'), j tsvector_ops (siglen='500'));
+CREATE INDEX concur_appclass_ind_2 on concur_appclass_tab
+ USING gist (i tsvector_ops, j tsvector_ops (siglen='300'));
+REINDEX TABLE CONCURRENTLY concur_appclass_tab;
+\d concur_appclass_tab
+DROP TABLE concur_appclass_tab;
-- Partitions
-- Create some partitioned tables
signature.asc
Description: PGP signature
Re: logical decoding/replication: new functions pg_ls_logicaldir and pg_ls_replslotdir
On Sat, Oct 23, 2021 at 11:10 PM Bharath Rupireddy
wrote:
>
> On Fri, Oct 22, 2021 at 9:26 PM Justin Pryzby wrote:
> > I concluded that it's better to add a function to list metadata of an
> > arbitrary
> > dir, rather than adding more functions to handle specific, hardcoded dirs:
> > https://www.postgresql.org/message-id/flat/20191227170220.ge12...@telsasoft.com
>
> I just had a quick look at the pg_ls_dir_metadata() patch(I didn't
> look at the other patches). While it's a good idea to have a single
> function for all the PGDATA directories, I'm not sure if one would
> ever need the info like type, change, creation path etc. If we do
> this, the function will become the linux equivalent command. I don't
> see the difference between modification and change time stamps. For
> debugging or analytical purposes in production environments, one would
> majorly look at the file name, it's size on the disk, modification
> time (to decide whether the file is stale or not, creation time (to
> decide how old is the file), file/directory(maybe?). I'm not sure if
> your patch has a recursive option for pg_ls_dir_metadata(), if it has,
> I think it's more complex from a usability perspective.
>
> And the functions like pg_ls_tmpdir, pg_ls_tmpdir, pg_ls_waldir etc.
> (existing) and pg_ls_logicaldir, pg_ls_replslotdir (yet to have) will
> provide the better usability compared to a generic function. Having
> said this, I don't oppose having a generic function returning the file
> name, file size, modification time, creation time, but not other info,
> please. If one is interested in knowing the other information file
> type, path etc. they can go run linux/windows/OS commands.
>
> In summary what I think at this point is:
> 1) pg_ls_logicaldir, pg_ls_replslotdir - better for usability and
> serving special purpose like their peers
I've added 3 functions pg_ls_logicalsnapdir, pg_ls_logicalmapdir,
pg_ls_replslotdir, and attached the patch. The sample output looks
like [1]. Please review it further.
Here's the CF entry - https://commitfest.postgresql.org/35/3390/
[1]
postgres=# select pg_ls_logicalsnapdir();
pg_ls_logicalsnapdir
---
(0-14A50C0.snap,128,"2021-10-30 09:15:56+00")
(0-14C46D8.snap,128,"2021-10-30 09:16:05+00")
(0-14C97C8.snap,132,"2021-10-30 09:16:20+00")
postgres=# select pg_ls_logicalmapdir();
pg_ls_logicalmapdir
---
(map-31d5-4eb-0_CDDDE88-2d9-2db,108,"2021-10-30 09:24:34+00")
(map-31d5-4eb-0_CDDDE88-2da-2db,108,"2021-10-30 09:24:34+00")
(map-31d5-4eb-0_CE48038-2dc-2de,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CE6BAF0-2dd-2df,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CD97DE0-2d9-2d9,36,"2021-10-30 09:24:30+00")
(map-31d5-4eb-0_CE24808-2da-2dd,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CE01200-2dc-2dc,36,"2021-10-30 09:24:34+00")
(map-31d5-4eb-0_CDDDE88-2db-2db,36,"2021-10-30 09:24:34+00")
(map-31d5-4eb-0_CE6BAF0-2dc-2df,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CDBA920-2d9-2da,108,"2021-10-30 09:24:32+00")
(map-31d5-4eb-0_CE01200-2da-2dc,108,"2021-10-30 09:24:34+00")
(map-31d5-4eb-0_CE6BAF0-2d9-2df,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CE24808-2db-2dd,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CE6BAF0-2db-2df,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CE24808-2dd-2dd,36,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CE24808-2dc-2dd,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CD74E48-2d8-2d8,36,"2021-10-30 09:24:25+00")
(map-31d5-4eb-0_CE24808-2d9-2dd,108,"2021-10-30 09:24:35+00")
postgres=# select pg_ls_replslotdir('mysub');
pg_ls_replslotdir
-
(xid-722-lsn-0-200.spill,36592640,"2021-10-30 09:18:29+00")
(xid-722-lsn-0-500.spill,4577860,"2021-10-30 09:18:32+00")
(state,200,"2021-10-30 09:18:25+00")
(xid-722-lsn-0-100.spill,25644220,"2021-10-30 09:18:29+00")
(xid-722-lsn-0-400.spill,36592640,"2021-10-30 09:18:32+00")
(xid-722-lsn-0-300.spill,36592640,"2021-10-30 09:18:32+00")
Regards,
Bharath Rupireddy.
v1-0001-Add-pg_ls_logicalsnapdir-pg_ls_logicalmapdir-and-.patch
Description: Binary data
Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY
On Sat, Oct 30, 2021 at 1:28 AM Michael Paquier wrote: > Hi all, > > While reviewing the code for opclass parameters with indexes, I have > noticed that opclass parameters are lost after a concurrent reindex. > As we use a IndexInfo to hold the information of the new index when > creating a copy of the old one, it is just a matter of making sure > that ii_OpclassOptions is filled appropriately, but that was missed by > 911e702. > > Attached is a patch to fix the issue. After a concurrent reindex, we > would not finish with a corrupted index, just with one rebuilt with > default opclass parameter values. > > Any objections or comments? > -- > Michael > Hi, + newInfo->ii_OpclassOptions = palloc0(sizeof(Datum) * +newInfo->ii_NumIndexAttrs); It seems we may not need to allocate these many entries (as shown by the concur_appclass_ind_2 example in the test). In the previous loop (starting line 1359), we can increment a counter which would finally tell us how many oldInfo->ii_OpclassOptions[i] is not NULL. Then that many entries can be allocated in the above code. Cheers
Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY
On Sat, Oct 30, 2021 at 3:59 AM Zhihong Yu wrote: > > > On Sat, Oct 30, 2021 at 1:28 AM Michael Paquier > wrote: > >> Hi all, >> >> While reviewing the code for opclass parameters with indexes, I have >> noticed that opclass parameters are lost after a concurrent reindex. >> As we use a IndexInfo to hold the information of the new index when >> creating a copy of the old one, it is just a matter of making sure >> that ii_OpclassOptions is filled appropriately, but that was missed by >> 911e702. >> >> Attached is a patch to fix the issue. After a concurrent reindex, we >> would not finish with a corrupted index, just with one rebuilt with >> default opclass parameter values. >> >> Any objections or comments? >> -- >> Michael >> > Hi, > > + newInfo->ii_OpclassOptions = palloc0(sizeof(Datum) * > +newInfo->ii_NumIndexAttrs); > > It seems we may not need to allocate these many entries (as shown by the > concur_appclass_ind_2 example in the test). > In the previous loop (starting line 1359), we can increment a counter > which would finally tell us how many oldInfo->ii_OpclassOptions[i] is not > NULL. > > Then that many entries can be allocated in the above code. > > Cheers > Hi, Upon further look, my previous comment was premature. Please ignore that. + newInfo->ii_OpclassOptions[i] = oldInfo->ii_OpclassOptions[i]; Should datumCopy() be used inside the loop ? I saw the following in get_attoptions(Oid relid, int16 attnum): result = datumCopy(attopts, false, -1); /* text[] */ Cheers
Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY
On Sat, Oct 30, 2021 at 04:11:06AM -0700, Zhihong Yu wrote: > Should datumCopy() be used inside the loop ? I saw the following > in get_attoptions(Oid relid, int16 attnum): Yeah, you are right that it would be better here to use get_attoptions() to grab a copy of each attribute's option directly from the catalogs. We also do that for predicates and expressions. -- Michael signature.asc Description: PGP signature
Re: Add additional information to src/test/ssl/README
Kevin Burke writes: > I've been trying to run the SSL tests against my branch and that was > tougher than expected because I didn't realize that additional output was > being saved that I couldn't see - it wasn't even getting to the part where > it could run the tests. This patch adds a note to the README explaining > where to look if you get an error. That's a generic thing about every one of our TAP tests, so documenting it in that one README doesn't seem like an appropriate answer. Would it have helped you to explain it in https://www.postgresql.org/docs/devel/regress-tap.html ? regards, tom lane
Re: Add additional information to src/test/ssl/README
I probably would not have looked there honestly; I was working in the terminal and had the source code right there. Could we put a link to that page in the README? "For more information on Postgres's TAP tests, see the docs: https://www.postgresql.org/docs/devel/regress-tap.html"; Kevin On Sat, Oct 30, 2021 at 7:14 AM Tom Lane wrote: > Kevin Burke writes: > > I've been trying to run the SSL tests against my branch and that was > > tougher than expected because I didn't realize that additional output was > > being saved that I couldn't see - it wasn't even getting to the part > where > > it could run the tests. This patch adds a note to the README explaining > > where to look if you get an error. > > That's a generic thing about every one of our TAP tests, so documenting > it in that one README doesn't seem like an appropriate answer. Would > it have helped you to explain it in > > https://www.postgresql.org/docs/devel/regress-tap.html > > ? > > regards, tom lane >
Re: Add additional information to src/test/ssl/README
Kevin Burke writes: > I probably would not have looked there honestly; I was working in the > terminal and had the source code right there. Yeah, that was kind of what I thought. > "For more information on Postgres's TAP tests, see the docs: > https://www.postgresql.org/docs/devel/regress-tap.html"; This doesn't seem tremendously useful, as we'd still have to duplicate it everywhere. A quick search finds these TAP suites that have associated README files: $ for ft in `find . -name t` > do > d=`dirname $ft` > if [ -e $d/README ]; then > echo $d/README > fi > done ./src/bin/pg_amcheck/README ./src/test/authentication/README ./src/test/kerberos/README ./src/test/ldap/README ./src/test/modules/test_misc/README ./src/test/modules/test_pg_dump/README ./src/test/modules/libpq_pipeline/README ./src/test/recovery/README ./src/test/ssl/README ./src/test/subscription/README The ones under modules/ can probably be excluded, as they're not there to provide usage directions; but that still leaves us with seven to touch. I'd be inclined to add just one sentence to the boilerplate text these use, along the lines of "If the tests fail, examining the logs left behind in tmp_check/log/ may be helpful." regards, tom lane
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Sat, 2021-10-30 at 13:24 +0530, Bharath Rupireddy wrote: > IMHO, moving away from SQL command "CHECKPOINT" to function > "pg_checkpoint()" isn't nice as the SQL command has been there for a > long time and all the applications or services that were/are being > built around the postgres ecosystem would have to adapt someday to > the > new function (if at all we deprecate the command and onboard the > function). This isn't good at all given the CHECKPOINT is one of the > mostly used commands in the apps or services layer. Moreover, if we > go > with the function pg_checkpoint(), we might see patches coming in for > pg_vacuum(), pg_reindex(), pg_cluster() and so on. I tend to agree with all of this. The CHECKPOINT command is already there and people already use it. If we are already chipping away at the need for superuser elsewhere, we should offer a way to use CHECKPOINT without being superuser. If the purpose[0] of predefined roles is that they allow you to do things that can't be expressed by GRANT, a predefined role pg_checkpointer seems to fit the bill. The main argument against[1] having a pg_checkpointer predefined role is that it creates a clutter of predefined roles. But it seems like just another part of the clutter of having a special SQL command merely for requesting a checkpoint. The alternative of creating a new function doesn't seem to alleviate the clutter. Some people will use the function and some will use the command, creating inconsistency in examples and scripts, and people will wonder which one is the "right" one. Regards, Jeff Davis [0] https://postgr.es/m/ca+tgmobqowzn62qwrx+oofjbphdubxytbeo-gsnpclvbhh4...@mail.gmail.com [1] https://postgr.es/m/8c661979-af85-4ae1-9e2b-2a091da3d...@amazon.com
Re: Add additional information to src/test/ssl/README
> On 30 Oct 2021, at 20:12, Tom Lane wrote: > I'd be inclined to add just one sentence to the boilerplate text these use, > along the lines of > "If the tests fail, examining the logs left behind in tmp_check/log/ > may be helpful." Wouldn't it make more sense to start collecting troubleshooting advice in src/test/perl/README and instead refer to that in the boilerplate? I notice that we don't document for example PG_TEST_NOCLEAN anywhere (which admittedly is my fault), a trubleshooting section in that file would be a good fit. -- Daniel Gustafsson https://vmware.com/
Re: Vulnerability identified with Postgres 13.4 for Windows
On Fri, Oct 29, 2021 at 10:40:06AM +, Joel Mariadasan (jomariad) wrote: > Hi, > > The scanning tool used by our organization has detected the presence of > vulnerable libxml version in the latest Postgres 13.4 release for windows > (Zip version). > > Detected by Automated Scanning tool: > libxml 2.9.10 > > Can you confirm if this is the same version of libxml used in Postgres? > We want to confirm if the detection is a false positive or a vulnerability. Joel: Could you provide the exact link for the postgres ZIP you used ? -- Justin
Re: Add additional information to src/test/ssl/README
Daniel Gustafsson writes: > Wouldn't it make more sense to start collecting troubleshooting advice in > src/test/perl/README and instead refer to that in the boilerplate? I notice > that we don't document for example PG_TEST_NOCLEAN anywhere (which admittedly > is my fault), a trubleshooting section in that file would be a good fit. Yeah, we could try to move all the repetitive stuff to there. I was a bit allergic to the idea of having README files refer to webpages, because you might be offline --- but referencing a different README doesn't have that issue. regards, tom lane
parallel vacuum comments
Hi, Due to bug #17245: [1] I spent a considerably amount of time looking at vacuum related code. And I found a few things that I think could stand improvement: - There's pretty much no tests. This is way way too complicated feature for that. If there had been tests for the obvious edge case of some indexes being too small to be handled in parallel, but others needing parallelism, the mistake leading to #17245 would have been caught during development. - There should be error check verifying that all indexes have actually been vacuumed. It's way too easy to have bugs leading to index vacuuming being skipped. - The state machine is complicated. It's very unobvious that an index needs to be processed serially by the leader if shared_indstats == NULL. - I'm very confused by the existance of LVShared->bitmap. Why is it worth saving 7 bits per index for something like this (compared to a simple array of bools)? Nor does the naming explain what it's for. The presence of the bitmap requires stuff like SizeOfLVShared(), which accounts for some of the bitmap size, but not all? But even though we have this space optimized bitmap thing, we actually need more memory allocated for each index, making this whole thing pointless. - Imo it's pretty confusing to have functions like lazy_parallel_vacuum_indexes() (in 13, renamed in 14) that "Perform index vacuum or index cleanup with parallel workers.", based on lps->lvshared->for_cleanup. - I don't like some of the new names introduced in 14. E.g. "do_parallel_processing" is way too generic. - On a higher level, a lot of this actually doesn't seem to belong into vacuumlazy.c, but should be somewhere more generic. Pretty much none of this code is heap specific. And vacuumlazy.c is large enough without the parallel code. Greetings, Andres Freund [1] https://postgr.es/m/17245-ddf06aaf85735f36%40postgresql.org
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 2021-Oct-30, Jeff Davis wrote: > I tend to agree with all of this. The CHECKPOINT command is already > there and people already use it. If we are already chipping away at the > need for superuser elsewhere, we should offer a way to use CHECKPOINT > without being superuser. +1 > If the purpose[0] of predefined roles is that they allow you to do > things that can't be expressed by GRANT, a predefined role > pg_checkpointer seems to fit the bill. +1 > The main argument against[1] having a pg_checkpointer predefined role > is that it creates a clutter of predefined roles. But it seems like > just another part of the clutter of having a special SQL command merely > for requesting a checkpoint. +1 -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ Y una voz del caos me habló y me dijo "Sonríe y sé feliz, podría ser peor". Y sonreí. Y fui feliz. Y fue peor.
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 10/30/21, 11:14 AM, "Jeff Davis" wrote: > On Sat, 2021-10-30 at 13:24 +0530, Bharath Rupireddy wrote: >> IMHO, moving away from SQL command "CHECKPOINT" to function >> "pg_checkpoint()" isn't nice as the SQL command has been there for a >> long time and all the applications or services that were/are being >> built around the postgres ecosystem would have to adapt someday to >> the >> new function (if at all we deprecate the command and onboard the >> function). This isn't good at all given the CHECKPOINT is one of the >> mostly used commands in the apps or services layer. Moreover, if we >> go >> with the function pg_checkpoint(), we might see patches coming in for >> pg_vacuum(), pg_reindex(), pg_cluster() and so on. > > I tend to agree with all of this. The CHECKPOINT command is already > there and people already use it. If we are already chipping away at the > need for superuser elsewhere, we should offer a way to use CHECKPOINT > without being superuser. I think Bharath brings up some good points. The simple fact is that CHECKPOINT has been around for a while, and creating functions for maintenance tasks would add just as much or more clutter than adding a predefined role for each one. I do wonder what we would've done if CHECKPOINT didn't already exist. Based on the goal of this thread, I get the feeling that we might've seriously considered introducing it as a function so that you can just GRANT EXECUTE as needed. Nathan
