Re: 2023-05-11 release announcement draft
Op 5/7/23 om 05:37 schreef Jonathan S. Katz: Attached is a draft of the release announcement for the upcoming update release on May 11, 2023. Please provide any suggestions, corrections, or notable omissions no later than 2023-05-11 0:00 AoE. 'leak in within a' should be 'leak within a' Erik
Re: Missing update of all_hasnulls in BRIN opclasses
Hi, On Sun, May 07, 2023 at 12:13:07AM +0200, Tomas Vondra wrote: > > c) ignore the issue - AFAICS this would be an issue only for (external) > code accessing BrinMemTuple structs, but I don't think we're aware of > any out-of-core BRIN opclasses or anything like that ... FTR there's at least postgis that implments BRIN opclasses (for geometries and geographies), but there's nothing fancy in the implementation and it doesn't access BrinMemTuple struct.
Re: MERGE lacks ruleutils.c decompiling support!?
I wrote: > I think we could probably commit this, though I've not tried it > in v15 yet. Ping? The hours grow short, if we're to get this into 15.3. regards, tom lane
2023-05-11 release announcement draft
Hi, Attached is a draft of the release announcement for the upcoming update release on May 11, 2023. Please provide any suggestions, corrections, or notable omissions no later than 2023-05-11 0:00 AoE. Thanks, Jonathan The PostgreSQL Global Development Group has released an update to all supported versions of PostgreSQL, including 15.3, 14.8, 13.11, 12.15, and 11.20. This release fixes over 80 bugs reported over the last several months. For the full list of changes, please review the [release notes](https://www.postgresql.org/docs/release/). PostgreSQL 11 EOL Notice PostgreSQL 11 will stop receiving fixes on November 9, 2023. If you are running PostgreSQL 11 in a production environment, we suggest that you make plans to upgrade to a newer, supported version of PostgreSQL. Please see our [versioning policy](https://www.postgresql.org/support/versioning/) for more information. Bug Fixes and Improvements -- This update fixes over 80 bugs that were reported in the last several months. The issues listed below affect PostgreSQL 15. Some of these issues may also affect other supported versions of PostgreSQL. Included in this release: * Several fixes for [`CREATE DATABASE`](https://www.postgresql.org/docs/current/sql-createdatabase.html) when using the `STRATEGY = WAL_LOG`, including a potential corruption that could lose modifications to a template/source database. * Fix crash with [`CREATE SCHEMA AUTHORIZATION`](https://www.postgresql.org/docs/current/sql-createschema.html). * Several fixes for [`MERGE`](https://www.postgresql.org/docs/current/sql-merge.html). * Several fixes for triggers in partitioned tables. * Disallow altering composite types that are stored in indexes. * Ensure that [`COPY TO`](https://www.postgresql.org/docs/current/sql-copy.html) from a parent table with [row-level security](https://www.postgresql.org/docs/current/ddl-rowsecurity.html) enabled does not copy any rows from child tables. * Adjust text-search-related character classification logic to correctly detect whether the prevailing locale is C when the default collation of a database uses the ICU provider. * Re-allow exponential notation in ISO-8601 interval fields. * Improve error reporting for various invalid JSON string literals. * Fix data corruption due to `vacuum_defer_cleanup_age` being larger than the current 64-bit xid. * Several fixes for the query parser and planner, including better detection of improperly-nested aggregates. * Fix partition pruning logic for partitioning on boolean columns when using a `IS NOT TRUE` condition. * Fix memory leak in [Memoize plan](https://www.postgresql.org/docs/current/runtime-config-query.html#GUC-ENABLE-MEMOIZE) execution. * Fix buffer refcount leak on foreign tables using partitions when performing batched inserts. * Restore support for sub-millisecond `vacuum_cost_delay` settings. * Several fixes for [views and rules](https://www.postgresql.org/docs/current/rules-views.html). * Avoid unnecessary work while scanning a multi-column [BRIN index](https://www.postgresql.org/docs/current/brin.html) with multiple scan keys. * Ignore dropped columns and generated columns during logical replication of an `UPDATE` or `DELETE` action. * Several fixes for naming and availability of wait events. * Support RSA-PSS certificates with [SCRAM-SHA-256](https://www.postgresql.org/docs/current/sasl-authentication.html#SASL-SCRAM-SHA-256) channel binding. This feature requires building with OpenSSL 1.1.1 or newer. * Avoid race condition with process ID tracking on Windows. * Fix memory leak in within a session for [PL/pgSQL](https://www.postgresql.org/docs/current/plpgsql.html) [`DO`](https://www.postgresql.org/docs/current/sql-do.html) blocks that use cast expressions. * Tighten array dimensionality checks from [PL/Perl](https://www.postgresql.org/docs/current/plperl.html) and [PL/Python](https://www.postgresql.org/docs/current/plpython.html) when converting list structures to multi-dimensional SQL arrays. * Fix [`pg_dump`](https://www.postgresql.org/docs/current/app-pgdump.html) so that partitioned tables that are hash-partitioned on an [enumerated type](https://www.postgresql.org/docs/current/datatype-enum.html) column can be restored successfully. * Fix for [`pg_trgm`](https://www.postgresql.org/docs/current/pgtrgm.html) where an unsatisfiable regular expression could lead to a crash when using a GiST or GIN index. * Limit memory usage of `pg_get_wal_records_info()` in [`pg_walinspect`](https://www.postgresql.org/docs/current/pgwalinspect.html). This release also updates time zone data files to tzdata release 2023c for DST law changes in Egypt, Greenland, Morocco, and Palestine. When observing Moscow time, Europe/Kirov and Europe/Volgograd now use the abbreviations MSK/MSD instead of numeric abbreviations, for consistency with other timezones observing Moscow time. Also, America/Yellowknife is no longer distinct from
Re: [PATCH] Add native windows on arm64 support
On Sat, May 06, 2023 at 12:35:40AM -0400, Tom Lane wrote: > Indeed, there's not much in this patch ... but it's impossible to see > how "run on an entirely new platform" isn't a new feature. Moreover, > it's a platform that very few of us will have any ability to support > or debug for. I can't see how it's a good idea to shove this in > post-feature-freeze. Let's plan to push it in when v17 opens. Fine by me. I have added an entry in the CF app to remember about this stuff as there was nothing present yet: https://commitfest.postgresql.org/43/4309/ -- Michael signature.asc Description: PGP signature
Re: Missing update of all_hasnulls in BRIN opclasses
On 4/24/23 23:20, Tomas Vondra wrote: > On 4/24/23 17:36, Alvaro Herrera wrote: >> On 2023-Apr-23, Tomas Vondra wrote: >> >>> here's an updated version of the patch, including a backport version. I >>> ended up making the code yet a bit closer to master by introducing >>> add_values_to_range(). The current pre-14 code has the loop adding data >>> to the BRIN tuple in two places, but with the "fixed" logic handling >>> NULLs and the empty_range flag the amount of duplicated code got too >>> high, so this seem reasonable. >> >> In backbranches, the new field to BrinMemTuple needs to be at the end of >> the struct, to avoid ABI breakage. >> Unfortunately, this is not actually possible :-( The BrinMemTuple has a FLEXIBLE_ARRAY_MEMBER at the end, so we can't place anything after it. I think we have three options: a) some other approach? - I really can't see any, except maybe for going back to the previous approach (i.e. encoding the info using the existing BrinValues allnulls/hasnulls flags) b) encoding the info in existing BrinMemTuple flags - e.g. we could use bt_placeholder to store two bits, not just one. Seems a bit ugly. c) ignore the issue - AFAICS this would be an issue only for (external) code accessing BrinMemTuple structs, but I don't think we're aware of any out-of-core BRIN opclasses or anything like that ... regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)
Hi! Thank you, Damir, for your patch. It is very interesting to review it! It seemed to me that the names of variables are not the same everywhere. I noticed that you used /ignore_datatype_errors_specified/ variable in /copy.c/ , but guc has a short name /ignore_datatype_errors/. Also you used the short variable name in /CopyFormatOptions/ structure. Name used /ignore_datatype_errors_specified /is seemed very long to me, may be use a short version of it (/ignore_datatype_errors/) in /copy.c/ too? Besides, I noticed that you used /ignored_errors/ variable in /CopyFromStateData/ structure and it's name is strikingly similar to name (/ignore_datatype_error//s/), but they have different meanings. Maybe it will be better to rename it as /ignored_errors_counter/? I tested last version /v5-0001-Add-new-COPY-option-IGNORE_DATATYPE_ERRORS.patch/ with /bytea/ data type and transaction cases. Eventually, I didn't find any problem there. I described my steps in more detail, if I missed something. *First of all, I ran copy function with IGNORE_DATATYPE_ERRORS parameter being in transaction block.* / //File t2.csv exists:/ |id,b 769,\ 1,\6e 2,\x5 5,\x| /Test:/ CREATE TABLE t (id INT , b BYTEA) ; postgres=# BEGIN; copy t FROM '/home/alena/postgres/t2.csv' WITH (format 'csv', IGNORE_DATATYPE_ERRORS, delimiter ',', HEADER); SAVEPOINT my_savepoint; BEGIN WARNING: invalid input syntax for type bytea WARNING: invalid input syntax for type bytea WARNING: invalid hexadecimal data: odd number of digits WARNING: 3 rows were skipped due to data type incompatibility COPY 1 SAVEPOINT postgres=*# copy t FROM '/home/alena/postgres/t2.csv' WITH (format 'csv', IGNORE_DATATYPE_ERRORS, delimiter ',', HEADER); WARNING: invalid input syntax for type bytea WARNING: invalid input syntax for type bytea WARNING: invalid hexadecimal data: odd number of digits WARNING: 3 rows were skipped due to data type incompatibility COPY 1 postgres=*# ROLLBACK TO my_savepoint; ROLLBACK postgres=*# select * from t; id | b + 5 | \x (1 row) postgres=*# copy t FROM '/home/alena/postgres/t2.csv' WITH (format 'csv', IGNORE_DATATYPE_ERRORS, delimiter ',', HEADER); WARNING: invalid input syntax for type bytea WARNING: invalid input syntax for type bytea WARNING: invalid hexadecimal data: odd number of digits WARNING: 3 rows were skipped due to data type incompatibility COPY 1 postgres=*# select * from t; id | b + 5 | \x 5 | \x (2 rows) postgres=*# commit; COMMIT *I tried to use the similar test and moved transaction block in function:* CREATE FUNCTION public.log2() RETURNS void LANGUAGE plpgsql SECURITY DEFINER AS $function$ BEGIN; copy t FROM '/home/alena/postgres/t2.csv' WITH (format 'csv', IGNORE_DATATYPE_ERRORS, delimiter ',', HEADER); SAVEPOINT my_savepoint; END; $function$; postgres=# delete from t; postgres=# select 1 as t from log2(); WARNING: invalid input syntax for type bytea WARNING: invalid input syntax for type bytea WARNING: invalid hexadecimal data: odd number of digits WARNING: 3 rows were skipped due to data type incompatibility t --- 1 (1 row) *Secondly I checked function copy with bytea datatype. * /t1.csv consists:/ id,b 769,\x2d 1,\x6e 2,\x5c 5,\x /And I ran it:/ postgres=# delete from t; DELETE 4 postgres=# copy t FROM '/home/alena/postgres/t2.csv' WITH (format 'csv', IGNORE_DATATYPE_ERRORS, delimiter ',', HEADER); WARNING: invalid input syntax for type bytea WARNING: invalid input syntax for type bytea WARNING: invalid hexadecimal data: odd number of digits WARNING: 3 rows were skipped due to data type incompatibility COPY 1 postgres=# select * from t; id | b + 5 | \x (1 row) -- --- Alena Rybakina Postgres Professional
Re: Remove duplicates of membership from results of \du
On Sat, May 6, 2023 at 6:37 AM Shinya Kato wrote: > Hi, hackers > > When executing \du, you can see duplicates of the same role in 'member of'. > This happens when admin | inherit | set options are granted by another > role. > There is already an ongoing patch discussing the needed changes to psql \du because of this change in tracking membership grant attributes. https://www.postgresql.org/message-id/flat/b9be2d0e-a9bc-0a30-492f-a4f68e4f7740%40postgrespro.ru David J.
Re: pg_stat_io not tracking smgrwriteback() is confusing
v5 attached. On Thu, May 4, 2023 at 12:44 PM Andres Freund wrote: > On 2023-04-27 11:36:49 -0400, Melanie Plageman wrote: > > > > /* and finally tell the kernel to write the data to > > > > storage */ > > > > reln = smgropen(currlocator, InvalidBackendId); > > > > smgrwriteback(reln, BufTagGetForkNum(), tag.blockNum, > > > > nblocks); > > > > Yes, as it is currently, IssuePendingWritebacks() is only used for shared > > buffers. My rationale for including IOObject is that localbuf.c calls > > smgr* functions and there isn't anything stopping it from calling > > smgrwriteback() or using WritebackContexts (AFAICT). > > I think it's extremely unlikely that we'll ever do that, because it's very > common to have temp tables that are bigger than temp_buffers. We basically > hope that the kernel can do good caching for us there. > > > > > Or I actually think we might not even need to pass around the io_* > > > parameters and could just pass immediate values to the > > > pgstat_count_io_op_time call. If we ever start using shared buffers > > > for thing other than relation files (for example SLRU?), we'll have to > > > consider the target individually for each buffer block. That being > > > said, I'm fine with how it is either. > > > > In IssuePendingWritebacks() we don't actually know which IOContext we > > are issuing writebacks for when we call pgstat_count_io_op_time() (we do > > issue pending writebacks for other IOContexts than IOCONTEXT_NORMAL). I > > agree IOObject is not strictly necessary right now. I've kept IOObject a > > member of WritebackContext for the reasons I mention above, however, I > > am open to removing it if it adds confusion. > > I don't think it's really worth adding struct members for potential future > safety. We can just add them later if we end up needing them. I've removed both members of WritebackContext and hard-coded IOOBJECT_RELATION in the call to pgstat_count_io_op_time(). > > From 7cdd6fc78ed82180a705ab9667714f80d08c5f7d Mon Sep 17 00:00:00 2001 > > From: Melanie Plageman > > Date: Mon, 24 Apr 2023 18:21:54 -0400 > > Subject: [PATCH v4] Add writeback to pg_stat_io > > > > 28e626bde00 added the notion of IOOps but neglected to include > > writeback. With the addition of IO timing to pg_stat_io in ac8d53dae5, > > the omission of writeback caused some confusion. Checkpointer write > > timing in pg_stat_io often differed greatly from the write timing > > written to the log. To fix this, add IOOp IOOP_WRITEBACK and track > > writebacks and writeback timing in pg_stat_io. > > For the future: It'd be good to note that catversion needs to be increased. Noted. I've added it to the commit message since I did a new version anyway. > > index 99f7f95c39..27b6f1a0a0 100644 > > --- a/doc/src/sgml/monitoring.sgml > > +++ b/doc/src/sgml/monitoring.sgml > > @@ -3867,6 +3867,32 @@ SELECT pid, wait_event_type, wait_event FROM > > pg_stat_activity WHERE wait_event i > > > > > > > > + > > + > > + > > +writebacks bigint > > + > > + > > +Number of units of size op_bytes which the > > backend > > +requested the kernel write out to permanent storage. > > + > > + > > + > > I think the reference to "backend" here is somewhat misplaced - it could be > checkpointer or bgwriter as well. We don't reference the backend in other > comparable columns of pgsio either... So, I tried to come up with something that doesn't make reference to any "requester" of the writeback and the best I could do was: "Number of units of size op_bytes requested that the kernel write out." This is awfully awkward sounding. "backend_type" is the name of the column in pg_stat_io. Client backends are always referred to as such in the pg_stat_io documentation. Thus, I think it is reasonable to use the word "backend" and assume people understand it could be any type of backend. However, since the existing docs for pg_stat_bgwriter use "backend" to mean "client backend", and I see a few uses of the word "process" in the stats docs, I've changed my use of the word "backend" to "process". > > diff --git a/src/backend/storage/buffer/buf_init.c > > b/src/backend/storage/buffer/buf_init.c > > index 0057443f0c..a7182fe95a 100644 > > --- a/src/backend/storage/buffer/buf_init.c > > +++ b/src/backend/storage/buffer/buf_init.c > > @@ -145,9 +145,15 @@ InitBufferPool(void) > > /* Init other shared buffer-management stuff */ > > StrategyInitialize(!foundDescs); > > > > - /* Initialize per-backend file flush context */ > > - WritebackContextInit(, > > - _flush_after); > > + /* > > + * Initialize per-backend file flush context. IOContext is > > initialized to > > + * IOCONTEXT_NORMAL because this is the most common context. IOObject > > is > > + * initialized to IOOBJECT_RELATION because writeback is currently > > only
Re: Add two missing tests in 035_standby_logical_decoding.pl
Hi, On 5/6/23 3:28 PM, Amit Kapila wrote: On Sat, May 6, 2023 at 1:52 PM Drouvot, Bertrand wrote: There is 2 runs with this extra info in place: A successful one: https://cirrus-ci.com/task/6528745436086272 A failed one: https://cirrus-ci.com/task/4558139312308224 Thanks, I think I got some clue as to why this test is failing randomly. Great, thanks! Observations: 1. In the failed run, the KeepLogSeg(), reduced the _logSegNo to 3 which is the reason for the failure because now the standby won't be able to remove/recycle the WAL file corresponding to segment number 3 which the test was expecting. Agree. 2. We didn't expect the KeepLogSeg() to reduce the _logSegNo because all logical slots were invalidated. However, I think we forgot that both standby and primary have physical slots which might also influence the XLogGetReplicationSlotMinimumLSN() calculation in KeepLogSeg(). Oh right... Next steps: = 1. The first thing is we should verify this theory by adding some LOG in KeepLogSeg() to see if the _logSegNo is reduced due to the value returned by XLogGetReplicationSlotMinimumLSN(). Yeah, will do that early next week. 2. The reason for the required file not being removed in the primary is also that it has a physical slot which prevents the file removal. Yeah, agree. But this one is not an issue as we are not checking for the WAL file removal on the primary, do you agree? 3. If the above theory is correct then I see a few possibilities to fix this test (a) somehow ensure that restart_lsn of the physical slot on standby is advanced up to the point where we can safely remove the required files; (b) just create a separate test case by initializing a fresh node for primary and standby where we only have logical slots on standby. This will be a bit costly but probably less risky. (c) any better ideas? (c): Since, I think, the physical slot on the primary is not a concern for the reason mentioned above, then instead of (b): What about postponing the physical slot creation on the standby and the cascading standby node initialization after this test? That way, this test would be done without a physical slot on the standby and we could also get rid of the "Wait for the cascading standby to catchup before removing the WAL file(s)" part. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Remove duplicates of membership from results of \du
Hi, hackers
When executing \du, you can see duplicates of the same role in 'member of'.
This happens when admin | inherit | set options are granted by another role.
---
postgres=# create role role_a login createrole;
CREATE ROLE
postgres=# \du
List of roles
Role name | Attributes | Member of
---++---
role_a | Create role
| {}
shinya | Superuser, Create role, Create DB, Replication, Bypass RLS
| {}
postgres=# set role role_a;
SET
postgres=> create role role_b;
CREATE ROLE
postgres=> \du
List of roles
Role name | Attributes | Member of
---++---
role_a | Create role
| {role_b}
role_b | Cannot login
| {}
shinya | Superuser, Create role, Create DB, Replication, Bypass RLS
| {}
postgres=> grant role_b to role_a;
GRANT ROLE
postgres=> \du
List of roles
Role name | Attributes | Member of
---++-
role_a | Create role
| {role_b,role_b}
role_b | Cannot login
| {}
shinya | Superuser, Create role, Create DB, Replication, Bypass RLS
| {}
postgres=> select rolname, oid from pg_roles where rolname = 'role_b';
rolname | oid
-+---
role_b | 16401
(1 row)
postgres=> select * from pg_auth_members where roleid = 16401;
oid | roleid | member | grantor | admin_option | inherit_option |
set_option
---+++-+--++
16402 | 16401 | 16400 | 10 | t | f | f
16403 | 16401 | 16400 | 16400 | f | t | t
(2 rows)
---
Attached patch resolves this issue.
Do you think?
Regards,
Shinya Kato
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 058e41e749..8aeb669100 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3632,7 +3632,7 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
"SELECT r.rolname, r.rolsuper, r.rolinherit,\n"
" r.rolcreaterole, r.rolcreatedb, r.rolcanlogin,\n"
" r.rolconnlimit, r.rolvaliduntil,\n"
- " ARRAY(SELECT b.rolname\n"
+ " ARRAY(SELECT DISTINCT b.rolname\n"
"FROM pg_catalog.pg_auth_members m\n"
"JOIN pg_catalog.pg_roles b ON (m.roleid = b.oid)\n"
"WHERE m.member = r.oid) as memberof");
Re: Add two missing tests in 035_standby_logical_decoding.pl
On Sat, May 6, 2023 at 1:52 PM Drouvot, Bertrand wrote: > > There is 2 runs with this extra info in place: > > A successful one: https://cirrus-ci.com/task/6528745436086272 > A failed one: https://cirrus-ci.com/task/4558139312308224 > Thanks, I think I got some clue as to why this test is failing randomly. Following is the comparison of successful and failed run logs for standby: Success case 2023-05-06 07:23:05.496 UTC [17617][walsender] [cascading_standby][3/0:0] DEBUG: 0: write 0/4000148 flush 0/400 apply 0/400 reply_time 2023-05-06 07:23:05.496365+00 2023-05-06 07:23:05.496 UTC [17617][walsender] [cascading_standby][3/0:0] LOCATION: ProcessStandbyReplyMessage, walsender.c:2101 2023-05-06 07:23:05.496 UTC [17617][walsender] [cascading_standby][3/0:0] DEBUG: 0: write 0/4000148 flush 0/4000148 apply 0/400 reply_time 2023-05-06 07:23:05.4964+00 2023-05-06 07:23:05.496 UTC [17617][walsender] [cascading_standby][3/0:0] LOCATION: ProcessStandbyReplyMessage, walsender.c:2101 2023-05-06 07:23:05.496 UTC [17617][walsender] [cascading_standby][3/0:0] DEBUG: 0: write 0/4000148 flush 0/4000148 apply 0/4000148 reply_time 2023-05-06 07:23:05.496531+00 2023-05-06 07:23:05.496 UTC [17617][walsender] [cascading_standby][3/0:0] LOCATION: ProcessStandbyReplyMessage, walsender.c:2101 2023-05-06 07:23:05.500 UTC [17706][client backend] [035_standby_logical_decoding.pl][2/12:0] LOG: 0: statement: checkpoint; 2023-05-06 07:23:05.500 UTC [17706][client backend] [035_standby_logical_decoding.pl][2/12:0] LOCATION: exec_simple_query, postgres.c:1074 2023-05-06 07:23:05.500 UTC [17550][checkpointer] LOG: 0: restartpoint starting: immediate wait ... ... 2023-05-06 07:23:05.500 UTC [17550][checkpointer] LOCATION: CheckPointReplicationSlots, slot.c:1576 2023-05-06 07:23:05.501 UTC [17550][checkpointer] DEBUG: 0: updated min recovery point to 0/4000148 on timeline 1 2023-05-06 07:23:05.501 UTC [17550][checkpointer] LOCATION: UpdateMinRecoveryPoint, xlog.c:2500 2023-05-06 07:23:05.515 UTC [17550][checkpointer] LOG: 0: CreateRestartPoint: After XLByteToSeg RedoRecPtr is 0/498, _logSegNo is 4 2023-05-06 07:23:05.515 UTC [17550][checkpointer] LOCATION: CreateRestartPoint, xlog.c:7271 2023-05-06 07:23:05.515 UTC [17550][checkpointer] LOG: 0: CreateRestartPoint: After KeepLogSeg RedoRecPtr is 0/498, endptr is 0/4000148, _logSegNo is 4 Failed case: == 2023-05-06 07:53:19.657 UTC [17914][walsender] [cascading_standby][3/0:0] DEBUG: 0: write 0/3D1A000 flush 0/3CFA000 apply 0/400 reply_time 2023-05-06 07:53:19.65207+00 2023-05-06 07:53:19.657 UTC [17914][walsender] [cascading_standby][3/0:0] LOCATION: ProcessStandbyReplyMessage, walsender.c:2101 2023-05-06 07:53:19.657 UTC [17914][walsender] [cascading_standby][3/0:0] DEBUG: 0: write 0/3D1A000 flush 0/3D1A000 apply 0/400 reply_time 2023-05-06 07:53:19.656471+00 2023-05-06 07:53:19.657 UTC [17914][walsender] [cascading_standby][3/0:0] LOCATION: ProcessStandbyReplyMessage, walsender.c:2101 ... ... 2023-05-06 07:53:19.686 UTC [17881][checkpointer] DEBUG: 0: updated min recovery point to 0/4000148 on timeline 1 2023-05-06 07:53:19.686 UTC [17881][checkpointer] LOCATION: UpdateMinRecoveryPoint, xlog.c:2500 2023-05-06 07:53:19.707 UTC [17881][checkpointer] LOG: 0: CreateRestartPoint: After XLByteToSeg RedoRecPtr is 0/498, _logSegNo is 4 2023-05-06 07:53:19.707 UTC [17881][checkpointer] LOCATION: CreateRestartPoint, xlog.c:7271 2023-05-06 07:53:19.707 UTC [17881][checkpointer] LOG: 0: CreateRestartPoint: After KeepLogSeg RedoRecPtr is 0/498, endptr is 0/4000148, _logSegNo is 3 Observations: 1. In the failed run, the KeepLogSeg(), reduced the _logSegNo to 3 which is the reason for the failure because now the standby won't be able to remove/recycle the WAL file corresponding to segment number 3 which the test was expecting. 2. We didn't expect the KeepLogSeg() to reduce the _logSegNo because all logical slots were invalidated. However, I think we forgot that both standby and primary have physical slots which might also influence the XLogGetReplicationSlotMinimumLSN() calculation in KeepLogSeg(). 3. Now, the reason for its success in some of the runs is that restart_lsn of physical slots also moved ahead by the time checkpoint happens. You can see the difference of LSNs for ProcessStandbyReplyMessage in failed and successful cases. Next steps: = 1. The first thing is we should verify this theory by adding some LOG in KeepLogSeg() to see if the _logSegNo is reduced due to the value returned by XLogGetReplicationSlotMinimumLSN(). 2. The reason for the required file not being removed in the primary is also that it has a physical slot which prevents the file removal. 3. If the above theory is correct then I see a few possibilities to fix this test (a) somehow ensure that restart_lsn of the physical slot on standby is advanced up to the point where we can
Re: Bancolombia Open Source Program Office - Proposal of contribution on lock inactive users
On 5/5/23 15:54, Francisco Luis Arbelaez Lopez wrote: If it matches your needs and objectives, I would like to receive more information related to the next steps of this contribution. If you want to contribute a patch for consideration, you would start by sending it here (to this list) with discussion about why it is needed, what problem it solves, how it is designed, how it performs, etc. Also register it for the next commitfest. See "What is a CommitFest?" here: https://www.postgresql.org/developer/ But first, I suggest you read through some or all of the following as well: https://wiki.postgresql.org/wiki/Development_information https://wiki.postgresql.org/wiki/Developer_FAQ#Getting_Involved https://wiki.postgresql.org/wiki/So,_you_want_to_be_a_developer%3F https://momjian.us/main/writings/pgsql/company_contributions.html Beyond that, you and/or some of the folks on your team should follow the discussions on this list to become familiar with the people and the ways of the community. Hope this helps, -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Questionable coding in nth_value
> On Sat, May 6, 2023 at 4:44 PM Tatsuo Ishii wrote: > >> Currently Window function nth_value is coded as following: >> >> nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, )); >> if (isnull) >> PG_RETURN_NULL(); >> const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1); >> >> if (nth <= 0) >> ereport(ERROR, >> : >> : >> >> Is there any reason why argument 'nth' is not checked earlier? >> IMO, it is more natural "if (nth <= 0)..." is placed right after "nth = >> DatumGetInt32...". >> >> Attached is the patch which does this. > > > Hmm, shouldn't we check if the argument of nth_value is null before we > check if it is greater than zero? So maybe we need to do this. That makes sense. I thought since this function is marked as strict, it would not be called if argument is NULL, but I was wrong. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
> The last time this was discussed ( > https://www.postgresql.org/message-id/1037735.1610402426%40sss.pgh.pa.us) > it was suggested to make the feature generalizable, beyond what the > standard says it should be limited to. I have read the mail. In my understanding nobody said that standard window functions should all accept the null treatment clause. Also Tom said: https://www.postgresql.org/message-id/5567.1537884439%40sss.pgh.pa.us > The questions of how we interface to the individual window functions > are really independent of how we handle the parsing problem. My > first inclination is to just pass the flags down to the window functions > (store them in WindowObject and provide some additional inquiry functions > in windowapi.h) and let them deal with it. As I said before I totally agree with this. With my patch if a (custom) window function does not want to accept null treatment clause, it just calls ErrorOutNullTreatment(). It will raise an error if IGNORE NULLS or RESPECT NULLS is provided. If it does call the function, it is up to the function how to deal with the null treatment. In another word, the infrastructure does not have fixed rules to allow/disallow null treatment clause for each window function. It's "delegated" to each window function. Anyway we can change the rule for other than nth_value etc. later easily once my patch is brought in. > With it generalizable, there would need to be extra checks for custom > functions, such as if they allow multiple column arguments (which I'll add > in v2 of the patch if the design's accepted). I am not sure if allowing-multiple-column-arguments patch should be provided with null-treatment patch. > So I think we need a consensus on whether to stick to limiting it to > several specific functions, or making it generalized yet agreeing the rules > to limit it (such as no agg functions, and no functions with multiple > column arguments). Let's see the discussion... Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Questionable coding in nth_value
On Sat, May 6, 2023 at 4:44 PM Tatsuo Ishii wrote:
> Currently Window function nth_value is coded as following:
>
> nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, ));
> if (isnull)
> PG_RETURN_NULL();
> const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);
>
> if (nth <= 0)
> ereport(ERROR,
> :
> :
>
> Is there any reason why argument 'nth' is not checked earlier?
> IMO, it is more natural "if (nth <= 0)..." is placed right after "nth =
> DatumGetInt32...".
>
> Attached is the patch which does this.
Hmm, shouldn't we check if the argument of nth_value is null before we
check if it is greater than zero? So maybe we need to do this.
--- a/src/backend/utils/adt/windowfuncs.c
+++ b/src/backend/utils/adt/windowfuncs.c
@@ -698,13 +698,14 @@ window_nth_value(PG_FUNCTION_ARGS)
nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, ));
if (isnull)
PG_RETURN_NULL();
- const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);
if (nth <= 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_ARGUMENT_FOR_NTH_VALUE),
errmsg("argument of nth_value must be greater than
zero")));
+ const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);
+
result = WinGetFuncArgInFrame(winobj, 0,
nth - 1, WINDOW_SEEK_HEAD, const_offset,
, NULL);
Thanks
Richard
Questionable coding in nth_value
Currently Window function nth_value is coded as following:
nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, ));
if (isnull)
PG_RETURN_NULL();
const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);
if (nth <= 0)
ereport(ERROR,
:
:
Is there any reason why argument 'nth' is not checked earlier?
IMO, it is more natural "if (nth <= 0)..." is placed right after "nth =
DatumGetInt32...".
Attached is the patch which does this.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
diff --git a/src/backend/utils/adt/windowfuncs.c b/src/backend/utils/adt/windowfuncs.c
index b87a624fb2..f4ff060930 100644
--- a/src/backend/utils/adt/windowfuncs.c
+++ b/src/backend/utils/adt/windowfuncs.c
@@ -696,15 +696,16 @@ window_nth_value(PG_FUNCTION_ARGS)
int32 nth;
nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, ));
- if (isnull)
- PG_RETURN_NULL();
- const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);
-
if (nth <= 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_ARGUMENT_FOR_NTH_VALUE),
errmsg("argument of nth_value must be greater than zero")));
+ if (isnull)
+ PG_RETURN_NULL();
+
+ const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);
+
result = WinGetFuncArgInFrame(winobj, 0,
nth - 1, WINDOW_SEEK_HEAD, const_offset,
, NULL);
Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
On Sat, 6 May 2023, 04:57 Tatsuo Ishii, wrote: > Attached is the patch to implement this (on top of your patch). > > test=# SELECT row_number() RESPECT NULLS OVER () FROM (SELECT 1) AS s; > ERROR: window function row_number cannot have RESPECT NULLS or IGNORE > NULLS > The last time this was discussed ( https://www.postgresql.org/message-id/1037735.1610402426%40sss.pgh.pa.us) it was suggested to make the feature generalizable, beyond what the standard says it should be limited to. With it generalizable, there would need to be extra checks for custom functions, such as if they allow multiple column arguments (which I'll add in v2 of the patch if the design's accepted). So I think we need a consensus on whether to stick to limiting it to several specific functions, or making it generalized yet agreeing the rules to limit it (such as no agg functions, and no functions with multiple column arguments).
Re: Add two missing tests in 035_standby_logical_decoding.pl
Hi, On 5/6/23 4:10 AM, Amit Kapila wrote: On Fri, May 5, 2023 at 7:53 PM Drouvot, Bertrand wrote: On 5/5/23 2:28 PM, Amit Kapila wrote: On Fri, May 5, 2023 at 5:36 PM Drouvot, Bertrand So, even on a successful test, we can see that the WAL file we expect to be removed on the standby has not been recycled on the primary before the test. Okay, one possibility of not removing on primary is that at the time of checkpoint (when we compute RedoRecPtr), the wal_swtich and insert is not yet performed because in that case it will compute the RedoRecPtr as a location before those operations which would be *3 file. However, it is not clear how is that possible except from a background checkpoint happening at that point but from LOGs, it appears that the checkpoint triggered by test has recycled the wal files. I think we need to add more DEBUG info to find that out. Can you please try to print 'RedoRecPtr', '_logSegNo', and recptr? Yes, will do. Okay, thanks, please try to print similar locations on standby in CreateRestartPoint(). The extra information is displayed that way: https://github.com/bdrouvot/postgres/commit/a3d6d58d105b379c04a17a1129bfb709302588ca#diff-c1cb3ab2a19606390c1a7ed00ffe5a45531702ca5faf999d401c548f8951c65bR6822-R6830 https://github.com/bdrouvot/postgres/commit/a3d6d58d105b379c04a17a1129bfb709302588ca#diff-c1cb3ab2a19606390c1a7ed00ffe5a45531702ca5faf999d401c548f8951c65bR7269-R7271 https://github.com/bdrouvot/postgres/commit/a3d6d58d105b379c04a17a1129bfb709302588ca#diff-c1cb3ab2a19606390c1a7ed00ffe5a45531702ca5faf999d401c548f8951c65bR7281-R7284 There is 2 runs with this extra info in place: A successful one: https://cirrus-ci.com/task/6528745436086272 A failed one: https://cirrus-ci.com/task/4558139312308224 For both the testrun.zip is available in the Artifacts section. Sharing this now in case you want to have a look (I'll have a look at them early next week on my side). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
