Side effect of remove_useless_groupby_columns
Hi, When looking at [1], I realized we may have a side effect when removing redundant columns in the GROUP BY clause. Suppose we have a query with ORDER BY 'b', and meanwhile column 'b' is also a group key. If we decide that 'b' is redundant due to being functionally dependent on other GROUP BY columns, we would remove it from group keys. This will make us lose the opportunity to leverage the index on 'b'. Here is an example for illustration. # create table t (a int primary key, b int); # insert into t select i, i%1000 from generate_series(1,100)i; # create index on t(b); By default, we remove 'b' from group keys and generate a plan as below: # explain (costs off) select b from t group by a, b order by b limit 10; QUERY PLAN Limit -> Sort Sort Key: b -> Group Group Key: a -> Index Scan using t_pkey on t (6 rows) The index on 'b' is not being used and we'll have to retrieve all the data underneath to perform the sort work. On the other hand, if we keep 'b' as a group column, we can get such a plan as: # explain (costs off) select b from t group by a, b order by b limit 10; QUERY PLAN - Limit -> Group Group Key: b, a -> Incremental Sort Sort Key: b, a Presorted Key: b -> Index Scan using t_b_idx on t (7 rows) With the help of 't_b_idx', we can avoid the full scan on 't' and it would run much faster. Any thoughts? [1] https://www.postgresql.org/message-id/flat/16869-26346b77d6ccaeec%40postgresql.org Thanks Richard
Re: NOT VALID for Unique Indexes
On Fri, 26 Feb 2021 at 17:36, Simon Riggs wrote: > On Mon, Jan 18, 2021 at 11:19 PM japin wrote: >> >> >> On Fri, 15 Jan 2021 at 00:22, Simon Riggs >> wrote: >> > As you may be aware the NOT VALID qualifier currently only applies to >> > CHECK and FK constraints, but not yet to unique indexes. I have had >> > customer requests to change that. >> > >> > It's a reasonably common requirement to be able to change an index >> > to/from a unique index, i.e. Unique -> NonUnique or NonUnique to >> > Unique. Previously, it was easy enough to do that using a catalog >> > update, but with security concerns and the fact that the optimizer >> > uses the uniqueness to optimize queries means that there is a gap in >> > our support. We obviously need to scan the index to see if it actually >> > can be marked as unique. >> > >> > In terms of locking we need to exclude writes while we add uniqueness, >> > so scanning the index to check it is unique would cause problems. So >> > we need to do the same thing as we do with other constraint types: add >> > the constraint NOT VALID in one transaction and then later validate it >> > in a separate transaction (if ever). >> > >> > I present a WIP patch to show it's a small patch to change Uniqueness >> > for an index, with docs and tests. >> > >> > ALTER INDEX SET [NOT] UNIQUE [NOT VALID] >> > ALTER INDEX VALIDATE UNIQUE >> > >> > It doesn't do the index validation scan (yet), but I wanted to check >> > acceptability, syntax and requirements before I do that. >> > >> > I can also add similar syntax for UNIQUE and PK constraints. >> > >> > Thoughts please? >> >> Great! I have some questions. >> >> 1. In the patch, you add a new attribute named "induniquevalid" in pg_index, >>however, there is a "indisvalid" in pg_index, can we use "indisvalid"? > > indisvalid already has defined meaning related to creating indexes > concurrently, so I was forced to create another column with a similar > name. > The doc of indisvalid says [1]: If true, the index is currently valid for queries. False means the index is possibly incomplete: it must still be modified by INSERT/UPDATE operations, but it cannot safely be used for queries. If it is unique, the uniqueness property is not guaranteed true either. So I think we can use it instead of create a new column. Does induniquevalid have any other special meaning? > Thanks for reviewing the code in detail. > >> 2. The foreign key and CHECK constraints are valid by using >>ALTER TABLE .. ADD table_constraint [ NOT VALID ] >>ALTER TABLE .. VALIDATE CONSTRAINT constraint_name >> >>Should we implement unique index valid/not valid same as foreign key and >>CHECK constraints? > > Yes, that is possible. (I wrote the NOT VALID patch for FKs, so was > aware of that). > > The syntax I presented was for ALTER INDEX. Not all UNIQUE indexes are > constraints, so it is important to add the option on ALTER INDEX. > Adding the ALTER TABLE syntax can be done later. > >> 3. If we use the syntax to valid/not valid the unique, should we support >>other constraints, such as foreign key and CHECK constraints? > > I'm sorry, I don't understand that question. FKs and CHECK constrants > are already supported, as you point out above. > I'm sorry, I mixed the indexes and constraints. [1] - https://www.postgresql.org/docs/devel/catalog-pg-index.html -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: [HACKERS] Custom compression methods
On my PC, this new change is causing a test failure:
SELECT SUBSTR(f1, 2000, 50) FROM cmdata1;
- substr
-
- 01234567890123456789012345678901234567890123456789
-(1 row)
-
+ERROR: compressed lz4 data is corrupt
@@ -119,15 +119,15 @@ lz4_cmdecompress_slice(const struct varlena *value, int32
slicelength)
int32 rawsize;
struct varlena *result;
- /* allocate memory for holding the uncompressed data */
- result = (struct varlena *) palloc(VARRAWSIZE_4B_C(value) + VARHDRSZ);
+ /* allocate memory for the uncompressed data */
+ result = (struct varlena *) palloc(slicelength + VARHDRSZ);
- /* decompress partial data using lz4 routine */
+ /* decompress the data */
rawsize = LZ4_decompress_safe_partial((char *) value +
VARHDRSZ_COMPRESS,
VARDATA(result),
VARSIZE(value) - VARHDRSZ_COMPRESS,
slicelength,
-
VARRAWSIZE_4B_C(value));
+
slicelength);
Also, in the tests, you have this at both the top and bottom of the file:
src/test/regress/sql/compression.sql:\set HIDE_COMPRESSAM false
src/test/regress/sql/compression.sql:\set HIDE_COMPRESSAM false
Whereas the patch I sent had at the end:
+\set HIDE_COMPRESSAM on
("on" is the default when run under pg_regress)
Re: regexp_positions()
Hi,
On Sun, Feb 28, 2021, at 03:13, David Fetter wrote:
> Maybe an int4multirange, which would fit unless I'm misunderstanding
> g's meaning with respect to non-overlapping patterns, but that might
> be a little too cute and not easy ever to extend.
>
> Come to that, would a row structure that looked like
>
> (match, start, end)
>
> be useful?
Nice, didn't know about the new multirange.
Its data structure seems like a perfect fit for this.
Hmm, I cannot find any catalog function to extract the ranges from the data
structure though?
As a caller, I might need the exact start/end values,
not just wanting to know if a certain values overlaps any of the ranges.
Is there such a function?
Here is a PoC that just returns the start_pos and end_pos for all the matches.
It would be simple to modify it to instead return multirange.
CREATE OR REPLACE FUNCTION regexp_ranges(string text, pattern text, OUT
start_pos integer, OUT end_pos integer)
RETURNS SETOF RECORD
LANGUAGE plpgsql
AS
$$
DECLARE
match text;
remainder text := string;
len integer;
BEGIN
end_pos := 0;
--
-- Ignore possible capture groups, instead just wrap the entire regex
-- in an enclosing capture group, which is then extracted as the first array
element.
--
FOR match IN SELECT (regexp_matches(string,format('(%s)',pattern),'g'))[1] LOOP
len := length(match);
start_pos := position(match in remainder) + end_pos;
end_pos := start_pos + len - 1;
RETURN NEXT;
remainder := right(remainder, -len);
END LOOP;
RETURN;
END
$$;
This works fine for small strings:
SELECT * FROM regexp_ranges(' aa aaa','a+');
start_pos | end_pos
---+-
1 | 4
6 | 7
10 | 12
(3 rows)
Time: 0.209 ms
But quickly gets slow for longer strings:
SELECT COUNT(*) FROM regexp_ranges(repeat(' aa aaa',1),'a+');
20001
Time: 98.663 ms
SELECT COUNT(*) FROM regexp_ranges(repeat(' aa aaa',2),'a+');
40001
Time: 348.027 ms
SELECT COUNT(*) FROM regexp_ranges(repeat(' aa aaa',3),'a+');
60001
Time: 817.412 ms
SELECT COUNT(*) FROM regexp_ranges(repeat(' aa aaa',4),'a+');
80001
Time: 1478.438 ms (00:01.478)
Compared to the much nicer observed O-notation for regexp_matches():
SELECT COUNT(*) FROM regexp_matches(repeat(' aa aaa',1),'(a+)','g');
20001
Time: 12.602 ms
SELECT COUNT(*) FROM regexp_matches(repeat(' aa aaa',2),'(a+)','g');
40001
Time: 25.161 ms
SELECT COUNT(*) FROM regexp_matches(repeat(' aa aaa',3),'(a+)','g');
60001
Time: 44.795 ms
SELECT COUNT(*) FROM regexp_matches(repeat(' aa aaa',4),'(a+)','g');
80001
Time: 57.292 ms
/Joel
Re: regexp_positions()
On Sat, Feb 27, 2021 at 08:51:27PM +0100, Joel Jacobson wrote: > Hi, > > Finding all matches in a string is convenient using regexp_matches() with the > 'g' flag. > > But if instead wanting to know the start and end positions of the occurrences, > one would have to first call regexp_matches(...,'g') to get all matches, > and then iterate through the results and search using strpos() and length() > repeatedly to find all start and end positions. > > Assuming regexp_matches() internally already have knowledge of the > occurrences, > maybe we could add a regexp_ranges() function that returns a two-dimensional > array, > with all the [[start,end], ...] positions? Maybe an int4multirange, which would fit unless I'm misunderstanding g's meaning with respect to non-overlapping patterns, but that might be a little too cute and not easy ever to extend. Come to that, would a row structure that looked like (match, start, end) be useful? Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: [PATCH] pgbench: Remove ecnt, a member variable of CState
On Fri, Feb 26, 2021 at 04:36:41PM -0300, Alvaro Herrera wrote: > +1 Thanks, done. -- Michael signature.asc Description: PGP signature
DETAIL for wrong scram password
When md5 password authentication fails, the server log file has a helpful detail to say why, usually one of: DETAIL: Role "none" does not exist. DETAIL: User "jjanes" has no password assigned. DETAIL: User "jjanes" has an expired password. DETAIL: Password does not match for user "jjanes". But for scram authentication, only the first three of these will be reported when applicable. If the password is simply incorrect, then you do get a DETAIL line reporting which line of pg_hba was used, but you don't get a DETAIL line reporting the reason for the failure. It is pretty unfriendly to make the admin guess what the absence of a DETAIL is supposed to mean. And as far as I can tell, this is not intentional. Note that in one case you do get the "does not match" line. That is if the user has a scram password assigned and the hba specifies plain-text 'password' as the method. So if the absence of the DETAIL is intentional, it is not internally consistent. The attached patch fixes the issue. I don't know if this is the correct location to be installing the message, maybe verify_client_proof should be doing it instead. I am also not happy to be testing 'doomed' here, but it was already checked a few lines up so I didn't want to go to lengths to avoid doing it here too. Cheers, Jeff scram_password_mismatch.patch Description: Binary data
Re: Bug in error reporting for multi-line JSON
Updated the patch based on feedback. The new status of this patch is: Needs review
Re: [BUG] segfault during delete
Thanks Amit for working on this fix! It seems correct to me, so I pushed it with trivial changes. Thanks Bertrand for reporting the problem. In addition to backpatching the code fix to pg12, I backpatched the test case to pg11. It worked fine for me (with no code changes), but it seems good to have it there just to make sure the buildfarm agrees with us on this.
Re: Extending range type operators to cope with elements
On Fri, Oct 30, 2020 at 11:08:19PM +0100, Tomas Vondra wrote: > Hi, > > + > + > +anyelement > anyrange > +boolean > + > + > +Is the element strictly right of the element? > + should say "of the range" ? > +++ b/src/backend/utils/adt/rangetypes.c > + /* An empty range is neither left nor right any other range */ > + /* An empty range is neither left nor right any element */ > + /* An empty range is neither left nor right any other range */ > + /* An empty range is neither left nor right any element */ > + /* An empty range is neither left nor right any element */ > + /* An empty range is neither left nor right any element */ I these comments should all say ".. left nor right OF any ..." -- Justin
regexp_positions()
Hi, Finding all matches in a string is convenient using regexp_matches() with the 'g' flag. But if instead wanting to know the start and end positions of the occurrences, one would have to first call regexp_matches(...,'g') to get all matches, and then iterate through the results and search using strpos() and length() repeatedly to find all start and end positions. Assuming regexp_matches() internally already have knowledge of the occurrences, maybe we could add a regexp_ranges() function that returns a two-dimensional array, with all the [[start,end], ...] positions? Some other databases have a singular regexp_position() function, that just returns the start positions for the first match. but I don't think such function adds much value, but if adding the pluralis one then maybe the singularis should be added as well, for completeness, since we have array_position() and array_positions(). I just wanted to share this idea now since there is currently a lot of other awesome work on the regex engine, and hear what others who are currently thinking a lot about regexes think of the idea. /Joel
Re: Allow matching whole DN from a client certificate
On Sat, Jan 30, 2021 at 04:18:12PM -0500, Andrew Dunstan wrote: > @@ -610,6 +610,19 @@ hostnogssenc database > user the verification of client certificates with any authentication > method that supports hostssl entries. > > + > + On any record using client certificate authentication, that is one > + using the cert authentication method or one > + using the clientcert option, you can specify I suggest instead of "that is" to instead parenthesize this part: | (one using the cert authentication method or the | clientcert option), you can specify > + which part of the client certificate credentials to match using > + the clientname option. This option can have one > + of two values. If you specify clientname=CN, which > + is the default, the username is matched against the certificate's > + Common Name (CN). If instead you specify > + clientname=DN the username is matched against the > + entire Distinguished Name (DN) of the certificate. > + This option is probably best used in comjunction with a username map. spell: conjunction
Re: [HACKERS] Custom compression methods
> Subject: [PATCH v28 3/4] Add default_toast_compression GUC This part isn't working. My first patch worked somewhat better: due to doing strcmp() with the default GUC, it avoided using the cached AM OID. (But it would've failed with more than 2 AMs, since the cache wasn't invalidated, since I couldn't tell when it was needed). Your patch does this: |postgres=# SET default_toast_compression=lz4 ; |postgres=# CREATE TABLE t(a text); |postgres=# \d+ t | a | text | | | | extended | pglz| | assign_default_toast_compression() should set default_toast_compression_oid=InvalidOid, rather than default_toast_compression=NULL. In my original patch, that was commented, since I was confused, not realizing that the GUC machinery itself assigns to the string value. We should assign to the cached Oid, instead. Reading my own patch, I see that in AccessMethodCallback() should also say InvalidOid. | default_toast_compression_oid = false; The false assignment was copied from namespace.c: baseSearchPathValid. -- Justin
Re: psql - add SHOW_ALL_RESULTS option
On 30.09.20 08:21, Michael Paquier wrote: On Mon, Jul 20, 2020 at 07:48:42AM +0200, Fabien COELHO wrote: Yes, indeed. I'm planning to investigate, hopefully this week. This reply was two months ago, and nothing has happened, so I have marked the patch as RwF. Given the ongoing work on returning multiple result sets from stored procedures[0], I went to dust off this patch. Based on the feedback, I put back the titular SHOW_ALL_RESULTS option, but set the default to on. I fixed the test failure in 013_crash_restart.pl. I also trimmed back the test changes a bit so that the resulting test output changes are visible better. (We could make those stylistic changes separately in another patch.) I'll put this back into the commitfest for another look. [0]: https://www.postgresql.org/message-id/flat/6e747f98-835f-2e05-cde5-86ee444a7...@2ndquadrant.com >From 22ac191084db1b6ab155202a09bc1a6fedde794f Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Sat, 27 Feb 2021 11:50:50 +0100 Subject: [PATCH v10] psql: Show all query results by default Author: Author: Fabien COELHO Discussion: https://www.postgresql.org/message-id/flat/alpine.DEB.2.21.1904132231510.8961@lancre --- .../expected/pg_stat_statements.out | 25 + doc/src/sgml/ref/psql-ref.sgml| 29 +- src/bin/psql/common.c | 639 ++ src/bin/psql/help.c | 2 + src/bin/psql/settings.h | 1 + src/bin/psql/startup.c| 10 + src/bin/psql/tab-complete.c | 2 +- src/test/regress/expected/copy2.out | 2 +- src/test/regress/expected/copyselect.out | 14 +- src/test/regress/expected/psql.out| 94 +++ src/test/regress/expected/transactions.out| 58 +- src/test/regress/sql/copyselect.sql | 4 +- src/test/regress/sql/psql.sql | 38 ++ src/test/regress/sql/transactions.sql | 2 +- 14 files changed, 609 insertions(+), 311 deletions(-) diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index 16158525ca..4ffb7e0076 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -50,8 +50,28 @@ BEGIN \; SELECT 2.0 AS "float" \; SELECT 'world' AS "text" \; COMMIT; + float +--- + 2.0 +(1 row) + + text +--- + world +(1 row) + -- compound with empty statements and spurious leading spacing \;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;; + ?column? +-- +6 +(1 row) + + ?column? +-- + ! +(1 row) + ?column? -- 5 @@ -61,6 +81,11 @@ COMMIT; SELECT 1 + 1 + 1 AS "add" \gset SELECT :add + 1 + 1 AS "add" \; SELECT :add + 1 + 1 AS "add" \gset + add +- + 5 +(1 row) + -- set operator SELECT 1 AS i UNION SELECT 2 ORDER BY i; i diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 13c1edfa4d..d14651adea 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -127,18 +127,11 @@ Options commands included in the string to divide it into multiple transactions. (See for more details about how the server handles multi-query strings.) - Also, psql only prints the - result of the last SQL command in the string. - This is different from the behavior when the same string is read from - a file or fed to psql's standard input, - because then psql sends - each SQL command separately. - Because of this behavior, putting more than one SQL command in a - single -c string often has unexpected results. - It's better to use repeated -c commands or feed - multiple commands to psql's standard input, + If having several commands executed in one transaction is not desired, + use repeated -c commands or feed multiple commands to + psql's standard input, either using echo as illustrated above, or via a shell here-document, for example: @@ -3523,10 +3516,6 @@ Meta-Commands commands included in the string to divide it into multiple transactions. (See for more details about how the server handles multi-query strings.) -psql prints only the last query result -it receives for each request; in this example, although all -three SELECTs are indeed executed, psql -only prints the 3. @@ -4102,6 +4091,18 @@ Variables +SHOW_ALL_RESULTS + + +When this variable is set to off, only the last +result of a combined (\;) query are shown instead +of all of them. Default is on. The off behavior +is for compatibility with older versions of psql. + + +
Re: repeated decoding of prepared transactions
On Sat, Feb 27, 2021 at 5:36 PM Amit Kapila wrote: > > On Sat, Feb 27, 2021 at 11:38 AM Amit Kapila wrote: > > > > On Fri, Feb 26, 2021 at 4:13 PM Ajin Cherian wrote: > > > > > > On Fri, Feb 26, 2021 at 7:47 PM Ajin Cherian wrote: > > > > > > > I've updated snapshot_was_exported_at_ member to pg_replication_slots > > > > as well. > > > > Do have a look and let me know if there are any comments. > > > > > > Update with both patches. > > > > > > > Thanks, I have made some minor changes to the first patch and now it > > looks good to me. The changes are as below: > > 1. Removed the changes related to exposing this new parameter via view > > as mentioned in my previous email. > > 2. Changed the variable name initial_consistent_point. > > 3. Ran pgindent, minor changes in comments, and modified the commit message. > > > > Let me know what you think about these changes. > > > > In the attached, I have just bumped SNAPBUILD_VERSION as we are > adding a new member in the SnapBuild structure. > Few minor comments: git am v6-0001-Avoid-repeated-decoding-of-prepared-transactions-.patch Applying: Avoid repeated decoding of prepared transactions after the restart. /home/vignesh/postgres/.git/rebase-apply/patch:286: trailing whitespace. #define SNAPBUILD_VERSION 4 warning: 1 line adds whitespace errors. There is one whitespace error. In commit a271a1b50e, we allowed decoding at prepare time and the prepare was decoded again if there is a restart after decoding it. It was done that way because we can't distinguish between the cases where we have not decoded the prepare because it was prior to consistent snapshot or we have decoded it earlier but restarted. To distinguish between these two cases, we have introduced an initial_consisten_point at the slot level which is an LSN at which we found a consistent point at the time of slot creation. One minor typo in commit message, initial_consisten_point should be initial_consistent_point Regards, Vignesh
Re: repeated decoding of prepared transactions
On Sat, Feb 27, 2021 at 8:29 AM Amit Kapila wrote: > > On Fri, Feb 26, 2021 at 7:26 PM vignesh C wrote: > > > > On Fri, Feb 26, 2021 at 4:13 PM Ajin Cherian wrote: > > > > > > On Fri, Feb 26, 2021 at 7:47 PM Ajin Cherian wrote: > > > > > > > I've updated snapshot_was_exported_at_ member to pg_replication_slots > > > > as well. > > > > Do have a look and let me know if there are any comments. > > > > > > Update with both patches. > > > > Thanks for fixing and providing an updated patch. Patch applies, make > > check and make check-world passes. I could see the issue working fine. > > > > Few minor comments: > > + snapshot_was_exported_at > > pg_lsn > > + > > + > > + The address (LSN) at which the logical > > + slot found a consistent point at the time of slot creation. > > + NULL for physical slots. > > + > > + > > > > > > I had seen earlier also we had some discussion on naming > > snapshot_was_exported_at. Can we change snapshot_was_exported_at to > > snapshot_exported_lsn, I felt if we can include the lsn in the name, > > the user will be able to interpret easily and also it will be similar > > to other columns in pg_replication_slots view. > > > > I have recommended above to change this name to initial_consistency_at > because there are times when we don't export snapshot and we still set > this like when creating slots with CRS_NOEXPORT_SNAPSHOT or when > creating via SQL APIs. I am not sure why Ajin neither changed the > name nor responded to that comment. What is your opinion? initial_consistency_at looks good to me. That is more understandable. Regards, Vignesh
Re: repeated decoding of prepared transactions
On Sat, Feb 27, 2021 at 11:38 AM Amit Kapila wrote:
>
> On Fri, Feb 26, 2021 at 4:13 PM Ajin Cherian wrote:
> >
> > On Fri, Feb 26, 2021 at 7:47 PM Ajin Cherian wrote:
> >
> > > I've updated snapshot_was_exported_at_ member to pg_replication_slots as
> > > well.
> > > Do have a look and let me know if there are any comments.
> >
> > Update with both patches.
> >
>
> Thanks, I have made some minor changes to the first patch and now it
> looks good to me. The changes are as below:
> 1. Removed the changes related to exposing this new parameter via view
> as mentioned in my previous email.
> 2. Changed the variable name initial_consistent_point.
> 3. Ran pgindent, minor changes in comments, and modified the commit message.
>
> Let me know what you think about these changes.
>
In the attached, I have just bumped SNAPBUILD_VERSION as we are
adding a new member in the SnapBuild structure.
> Next, I'll review your second patch which allows the 2PC option to be
> enabled only at slot creation time.
>
Few comments on 0002 patch:
=
1.
+
+ /*
+ * Disable two-phase here, it will be set in the core if it was
+ * enabled whole creating the slot.
+ */
+ ctx->twophase = false;
Typo, /whole/while. I think we don't need to initialize this variable
here at all.
2.
+ /* If twophase is set on the slot at create time, then
+ * make sure the field in the context is also updated
+ */
+ if (MyReplicationSlot->data.twophase)
+ {
+ ctx->twophase = true;
+ }
+
For multi-line comments, the first line of comment should be empty.
Also, I think this is not the right place because the WALSender path
needs to set it separately. I guess you can set it in
CreateInitDecodingContext/CreateDecodingContext by doing something
like
ctx->twophase &= MyReplicationSlot->data.twophase
3. I think we can support this option at the protocol level in a
separate patch where we need to allow it via replication commands (say
when we support it in CreateSubscription). Right now, there is nothing
to test all the code you have added in repl_gram.y.
4. I think we can expose this new option via pg_replication_slots.
--
With Regards,
Amit Kapila.
v6-0001-Avoid-repeated-decoding-of-prepared-transactions-.patch
Description: Binary data
v6-0002-Add-option-to-enable-two-phase-commits-in-pg_crea.patch
Description: Binary data
Re: Tid scan improvements
On Fri, 19 Feb 2021 at 20:37, David Rowley wrote: > > On Thu, 18 Feb 2021 at 09:45, David Rowley wrote: > > > > On Wed, 17 Feb 2021 at 11:05, Andres Freund wrote: > > > How does this interact with rescans? > > > > We must call table_rescan() before calling table_set_tidrange() again. > > That perhaps could be documented better. I'm just unsure if that > > should be documented in tableam.h or if it's a restriction that only > > needs to exist in heapam.c > > I've changed things around so that we no longer explicitly call > table_rescan() in nodeTidrangescan.c. Instead table_set_tidrange() > does a rescan call. I also adjusted the documentation to mention that > changing the tid range starts the scan again. This does mean we'll do > a ->scan_rescan() the first time we do table_set_tidrange(). I'm not > all that sure that matters. I've pushed this now. I did end up changing the function name in tableam.h so that we no longer expose the table_set_tidrange(). Instead, the range is set by either table_beginscan_tidrange() or table_rescan_tidrange(). There's no need to worry about what would happen if someone were to change the TID range mid-scan. Apart from that, I adjusted a few comments and changed the regression tests a little to get rid of the tidrangescan_empty table. This was created to ensure empty tables work correctly. Instead, I just did those tests before populating the tidrangescan table. This just makes the test run a little faster since we're creating and dropping 1 less table. David
Re: [PATCH] Note effect of max_replication_slots on subscriber side in documentation.
On Sat, Feb 27, 2021 at 2:47 AM Paul Martinez wrote: > > On Fri, Feb 26, 2021 at 5:22 AM Amit Kapila wrote: > > > > https://www.postgresql.org/docs/devel/logical-replication-config.html > > > > Ah, yep. I added a clause to the end of the sentence to clarify why we're > using max_replication_slots here: > > - The subscriber also requires the max_replication_slots to be set. > > + The subscriber also requires that max_replication_slots be set to > + configure how many replication origins can be tracked. > LGTM. > > > > Okay, that makes sense. However, I have sent a patch today (see [1]) > > where I have slightly updated the subscriber-side configuration > > paragraph. From PG-14 onwards, table synchronization workers also use > > origins on subscribers, so you might want to adjust. > > > > ... > > > > I guess we can leave adding GUC to some other day as that might > > require a bit broader acceptance and we are already near to the start > > of last CF. I think we can still consider it if we few more people > > share the same opinion as yours. > > > > Great. I'll wait to update the GUC patch until your patch and/or my > doc-only patch get merged. Should I add it to the March CF? > Which patch are you asking about doc-patch or GUC one? If you are asking for a doc-patch, then I don't think it is required, I'll take care of this sometime next week. For the GUC patch, my suggestion would be to propose for v15 with an appropriate use-case. At this point (just before the last CF of release), people are mostly busy with patches that are going on for a long time so this might not get due attention unless few people show-up and say it is important. However, it is up to you, if you want feel free to register your GUC patch in the upcoming CF. > Separate question: are documentation updates like these ever backported > to older versions that are still supported? > Not every doc-change is back-ported but I think it is good to backport the user-visible ones. It is on a case-by-case basis. For this, I think we can backport unless you or others feel otherwise? > And if so, would the changes > be reflected immediately, or would they require a minor point release? > Where you are referring to the docs? If you are checking from code, it will be reflected immediately. -- With Regards, Amit Kapila.
Re: Remove latch.c workaround for Linux < 2.6.27
On Sat, Feb 27, 2021 at 9:01 PM Heikki Linnakangas wrote: > On 27 February 2021 01:10:23 EET, Thomas Munro wrote: > >Commit 82ebbeb0 added a workaround for (already in 2017) ancient Linux > >kernels with no EPOLL_CLOEXEC. I don't see any such systems in the > >build farm today (and if there is one hiding in there somewhere, it's > >well past time to upgrade). I'd like to rip that code out, because > >I'm about to commit some new code that uses another 2.6.17+ > >XXX_CLOEXEC flag, and it'd be silly to have to write new workaround > >code for that too, and a contradiction to have fallback code in one > >place but not another. Any objections? > > What happens if you try to try to compile or run on such an ancient kernel? > Does it fall back to something else? Can you still make it work with > different configure options? > > I'm just curious, not objecting. With this patch, I guess you'd have to define WAIT_USE_POLL. I suppose we could fall back to that automatically if EPOLL_CLOEXEC isn't defined, if anyone thinks that's worth bothering with. Even though Linux < 2.6.17 is not relevant, one thing I have wondered about is what other current OSes might have copied Linux's epoll API and get me into trouble by being incomplete. So far I have found only illumos, based on googling about OSes that are in our build farm (my idea of what OSes we support in some sense), and BF animal damselfly's configure output seems to confirm that it does have it. Googling tells me that it does seem to have the full modern version of the API, so fingers crossed (looks like it also has signalfd() too, which is interesting for my latch optimisation patch which assumes that if you have epoll you also have signalfd).
Shared memory size computation oversight?
Hi,
While investigating on some strange "out of shared memory" error reported on
the french BBS [1], I noticed that 09adc9a8c09 (adding Robert in Cc) changed
ShmemAlloc alignment to CACHELINEALIGN but didn't update any related code that
depended on it.
Most of the core code isn't impacted as it doesn't have to reserve any shared
memory, but AFAICT pg_prewarm and pg_stat_statements can now slightly
underestimate the amount of shared memory they'll use, and similarly for any
third party extension that still rely on MAXALIGN alignment. As a consequence
those extension can consume a few hundred bytes more than they reserved, which
probably will be borrowed from the lock hashtables reserved size that isn't
alloced immediately. This can later lead to ShmemAlloc failing when trying to
acquire a lock while the hash table is almost full but should still have enough
room to store it, which could explain the error reported.
PFA a patch that fixes pg_prewarm and pg_stat_statements explicit alignment to
CACHELINEALIGN, and also update the alignment in hash_estimate_size() to what I
think ShmemInitHash will actually consume.
[1] https://forums.postgresql.fr/viewtopic.php?pid=32138#p32138 sorry, it's all
in French.
>From 59f4959f3b8f7e69fdbe88a85efaca80b6718a38 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Sat, 27 Feb 2021 15:45:29 +0800
Subject: [PATCH v1] Fix various shared memory computation
Oversight in 09adc9a8c09.
---
contrib/pg_prewarm/autoprewarm.c| 2 +-
contrib/pg_stat_statements/pg_stat_statements.c | 2 +-
src/backend/utils/hash/dynahash.c | 8
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index b3f73ea92d..887e68b288 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -138,7 +138,7 @@ _PG_init(void)
EmitWarningsOnPlaceholders("pg_prewarm");
- RequestAddinShmemSpace(MAXALIGN(sizeof(AutoPrewarmSharedState)));
+ RequestAddinShmemSpace(CACHELINEALIGN(sizeof(AutoPrewarmSharedState)));
/* Register autoprewarm worker, if enabled. */
if (autoprewarm)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 62cccbfa44..4e045ffba7 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1933,7 +1933,7 @@ pgss_memsize(void)
{
Size size;
- size = MAXALIGN(sizeof(pgssSharedState));
+ size = CACHELINEALIGN(sizeof(pgssSharedState));
size = add_size(size, hash_estimate_size(pgss_max, sizeof(pgssEntry)));
return size;
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 6546e3c7c7..e532a3825a 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -797,19 +797,19 @@ hash_estimate_size(long num_entries, Size entrysize)
nDirEntries <<= 1; /* dir_alloc doubles dsize at each call */
/* fixed control info */
- size = MAXALIGN(sizeof(HASHHDR)); /* but not HTAB, per above */
+ size = CACHELINEALIGN(sizeof(HASHHDR)); /* but not HTAB, per above */
/* directory */
size = add_size(size, mul_size(nDirEntries, sizeof(HASHSEGMENT)));
/* segments */
size = add_size(size, mul_size(nSegments,
- MAXALIGN(DEF_SEGSIZE * sizeof(HASHBUCKET;
+ CACHELINEALIGN(DEF_SEGSIZE * sizeof(HASHBUCKET;
/* elements --- allocated in groups of choose_nelem_alloc() entries */
elementAllocCnt = choose_nelem_alloc(entrysize);
nElementAllocs = (num_entries - 1) / elementAllocCnt + 1;
elementSize = MAXALIGN(sizeof(HASHELEMENT)) + MAXALIGN(entrysize);
size = add_size(size,
- mul_size(nElementAllocs,
- mul_size(elementAllocCnt, elementSize)));
+ CACHELINEALIGN(Nmul_size(nElementAllocs,
+ mul_size(elementAllocCnt, elementSize;
return size;
}
--
2.30.1
Re: Remove latch.c workaround for Linux < 2.6.27
On 27 February 2021 01:10:23 EET, Thomas Munro wrote: >Hello, > >Commit 82ebbeb0 added a workaround for (already in 2017) ancient Linux >kernels with no EPOLL_CLOEXEC. I don't see any such systems in the >build farm today (and if there is one hiding in there somewhere, it's >well past time to upgrade). I'd like to rip that code out, because >I'm about to commit some new code that uses another 2.6.17+ >XXX_CLOEXEC flag, and it'd be silly to have to write new workaround >code for that too, and a contradiction to have fallback code in one >place but not another. Any objections? What happens if you try to try to compile or run on such an ancient kernel? Does it fall back to something else? Can you still make it work with different configure options? I'm just curious, not objecting. - Heikki
