Re: SQL Property Graph Queries (SQL/PGQ)
Hi I am attaching a new patch for a minor feature addition. - Adding support for 'Labels and properties: EXCEPT list' Please let me know if something is missing. Thanks and Regards Imran Zaheer On Mon, Jul 22, 2024 at 9:02 PM Ashutosh Bapat wrote: > > On Wed, Jul 17, 2024 at 11:04 AM Ashutosh Bapat > wrote: > > > > On Mon, Jul 8, 2024 at 7:07 PM Ashutosh Bapat > > wrote: > > > > > > > > > > > > On Thu, Jun 27, 2024 at 6:01 PM Peter Eisentraut > > > wrote: > > >> > > >> Here is a new version of this patch. I have been working together with > > >> Ashutosh on this. While the version 0 was more of a fragile demo, this > > >> version 1 has a fairly complete minimal feature set and should be useful > > >> for playing around with. We do have a long list of various internal > > >> bits that still need to be fixed or revised or looked at again, so there > > >> is by no means a claim that everything is completed. > > > > > > > > > PFA the patchset fixing compilation error reported by CI bot. > > > 0001 - same as previous one > > > 0002 - fixes compilation error > > > 0003 - adds support for WHERE clause in graph pattern missing in the > > > first patch. > > > > > > > There's a test failure reported by CI. Property graph related tests > > are failing when regression is run from perl tests. The failure is > > reported only on Free BSD. > > I thought it's related to FreeBSD but the bug could be observed > anywhere with -DRELCACHE_FORCE_RELEASE. It's also reported indirectly > by valgrind. > > When infering properties of an element from the underlying table's > attributes, the attribute name pointed to the memory in the heap tuple > of pg_attribute row. Thus when the tuple was released, it pointed to a > garbage instead of actual column name resulting in column not found > error. > > Attached set of patches with an additional patch to fix the bug. > > 0001 - same as previous one > 0002 - fixes pgperltidy complaints > 0003 - fixes compilation failure > 0004 - fixes issue seen on CI > 0005 - adds support for WHERE clause in graph pattern missing in the > first patch. > > Once reviewed, patches 0002 to 0005 should be merged into 0001. > > -- > Best Wishes, > Ashutosh Bapat 0006-Support-for-EXCEPT-list-in-Labels-and-properties.patch Description: Binary data
RE: Conflict detection and logging in logical replication
On Friday, July 26, 2024 2:26 PM Amit Kapila wrote:
>
> On Fri, Jul 26, 2024 at 9:39 AM shveta malik wrote:
> >
> > On Thu, Jul 11, 2024 at 7:47 AM Zhijie Hou (Fujitsu)
> > wrote:
> > >
> > > On Wednesday, July 10, 2024 5:39 PM shveta malik
> wrote:
> > > >
> >
> > > > 2)
> > > > Another case which might confuse user:
> > > >
> > > > CREATE TABLE t1 (pk integer primary key, val1 integer, val2
> > > > integer);
> > > >
> > > > On PUB: insert into t1 values(1,10,10); insert into t1
> > > > values(2,20,20);
> > > >
> > > > On SUB: update t1 set pk=3 where pk=2;
> > > >
> > > > Data on PUB: {1,10,10}, {2,20,20}
> > > > Data on SUB: {1,10,10}, {3,20,20}
> > > >
> > > > Now on PUB: update t1 set val1=200 where val1=20;
> > > >
> > > > On Sub, I get this:
> > > > 2024-07-10 14:44:00.160 IST [648287] LOG: conflict update_missing
> > > > detected on relation "public.t1"
> > > > 2024-07-10 14:44:00.160 IST [648287] DETAIL: Did not find the row
> > > > to be updated.
> > > > 2024-07-10 14:44:00.160 IST [648287] CONTEXT: processing remote
> > > > data for replication origin "pg_16389" during message type
> > > > "UPDATE" for replication target relation "public.t1" in
> > > > transaction 760, finished at 0/156D658
> > > >
> > > > To user, it could be quite confusing, as val1=20 exists on sub but
> > > > still he gets update_missing conflict and the 'DETAIL' is not
> > > > sufficient to give the clarity. I think on HEAD as well (have not
> > > > tested), we will get same behavior i.e. update will be ignored as
> > > > we make search based on RI (pk in this case). So we are not
> > > > worsening the situation, but now since we are detecting conflict, is it
> possible to give better details in 'DETAIL' section indicating what is
> actually
> missing?
> > >
> > > I think It's doable to report the row value that cannot be found in
> > > the local relation, but the concern is the potential risk of
> > > exposing some sensitive data in the log. This may be OK, as we are
> > > already reporting the key value for constraints violation, so if
> > > others also agree, we can add the row value in the DETAIL as well.
> >
> > This is still awaiting some feedback. I feel it will be good to add
> > some pk value at-least in DETAIL section, like we add for other
> > conflict types.
> >
>
> I agree that displaying pk where applicable should be okay as we display it at
> other places but the same won't be possible when we do sequence scan to
> fetch the required tuple. So, the message will be different in that case,
> right?
After some research, I think we can report the key values in DETAIL if the
apply worker uses any unique indexes to find the tuple to update/delete.
Otherwise, we can try to output all column values in DETAIL if the current user
of apply worker has SELECT access to these columns.
This is consistent with what we do when reporting table constraint violation
(e.g. when violating a check constraint, it could output all the column value
if the current has access to all the column):
- First, use super user to create a table.
CREATE TABLE t1 (c1 int, c2 int, c3 int check (c3 < 5));
- 1) using super user to insert a row that violates the constraint. We should
see all the column value.
INSERT INTO t1(c3) VALUES (6);
ERROR: new row for relation "t1" violates check constraint
"t1_c3_check"
DETAIL: Failing row contains (null, null, 6).
- 2) use a user without access to all the columns. We can only see the inserted
column and
CREATE USER regress_priv_user2;
GRANT INSERT (c1, c2, c3) ON t1 TO regress_priv_user2;
SET SESSION AUTHORIZATION regress_priv_user2;
INSERT INTO t1 (c3) VALUES (6);
ERROR: new row for relation "t1" violates check constraint
"t1_c3_check"
DETAIL: Failing row contains (c3) = (6).
To achieve this, I think we can expose the ExecBuildSlotValueDescription
function and use it in conflict reporting. What do you think ?
Best Regards,
Hou zj
Re: PG 17 and GUC variables
On 04/08/2024 06:29, Robert Treat wrote: I was looking at trace_connection_negotiation and ran across this commit removing it's mention from the release notes because it is undocumented: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=95cabf542f04b634303f899600ea62fb256a08c2 Why is the right solution to remove it from the release notes rather than to document it properly? It's not like people won't notice a new GUC has popped up in their configs. Also, presumaing I'm unerstanding it's purpose correctly, ISTM it would fit along side other trace_* gucs in https://www.postgresql.org/docs/current/runtime-config-developer.html#RUNTIME-CONFIG-DEVELOPER. Not sure whether it's worth mentioning in release notes, but I think you're right that it should be listed in that docs section. How about the attached description? I see that there are two more developer-only GUCs that are not listed in the docs: trace_syncscan optimize_bounded_sort There's a comment on them that says "/* this is undocumented because not exposed in a standard build */", but that seems like a weak reason, given that many of the other options in that docs section also require additional build-time options. I think we should add those to the docs too for the sake of completeness. -- Heikki Linnakangas Neon (https://neon.tech) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index a65839a6709..2f0ba3858a4 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -11667,6 +11667,26 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' + + trace_connection_negotiation (boolean) + + trace_connection_negotiation configuration parameter + + + + +If on, write information to the log about encryption negotiation +packets on each client connection. This can be useful for debugging +client connectivity issues from the server side. The log messages do +not contain sensitive information like encryption keys. + + +This parameter can only be set in the +postgresql.conf file or on the server command line. + + + + trace_notify (boolean)
Re: UUID v7
> On 28 Jul 2024, at 23:44, Andrey M. Borodin wrote: > > PFA version accepting offset interval. There was a bug: when time was not moving on, I was updating used time by a nanosecond, instead of 1/4096 of millisecond. V27 fixes that. Thanks! Best regards, Andrey Borodin. v27-0001-Implement-UUID-v7.patch Description: Binary data
Re: Official devcontainer config
On 2024-08-03 Sa 10:13 PM, Junwang Zhao wrote: On Sat, Aug 3, 2024 at 7:30 PM Andrew Dunstan wrote: On 2024-08-02 Fr 2:45 PM, Peter Eisentraut wrote: On 01.08.24 23:38, Andrew Dunstan wrote: Not totally opposed, and I will probably give it a try very soon, but I'm wondering if this really needs to go in the core repo. We've generally shied away from doing much in the way of editor / devenv support, trying to be fairly agnostic. It's true we carry .dir-locals.el and .editorconfig, so that's not entirely true, but those are really just about supporting our indentation etc. standards. Yeah, the editor support in the tree ought to be minimal and factual, based on coding standards and widely recognized best practices, not a collection of one person's favorite aliases and scripts. If the scripts are good, let's look at them and maybe put them under src/tools/ for everyone to use. But a lot of this looks like it will requite active maintenance if output formats or node formats or build targets etc. change. And other things require specific local paths. That's fine for a local script or something, but not for a mainline tool that the community will need to maintain. I suggest to start with a very minimal configuration. What are the settings that absolute everyone will need, maybe to set indentation style or something. I believe you can get VS Code to support editorconfig, so from that POV maybe we don't need to do anything. I did try yesterday with the code from the OP's patch symlinked into my repo, but got an error with the Docker build, which kinda reinforces your point. The reason symlink does not work is that configure_vscode needs to copy launch.json and tasks.json into .vscode, it has to be in the WORKDIR/.devcontainer. That's kind of awful. Anyway, I think we don't need to do anything about ignoring those. The user should simply add entries for them to .git/info/exclude or their local global exclude file (I have core.excludesfile = /home/andrew/.gitignore set.) I was eventually able to get it to work without using a symlink. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: PG 17 and GUC variables
On Sun, Aug 4, 2024 at 4:45 AM Heikki Linnakangas wrote: > On 04/08/2024 06:29, Robert Treat wrote: > > I was looking at trace_connection_negotiation and ran across this > > commit removing it's mention from the release notes because it is > > undocumented: > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=95cabf542f04b634303f899600ea62fb256a08c2 > > > > Why is the right solution to remove it from the release notes rather > > than to document it properly? It's not like people won't notice a new > > GUC has popped up in their configs. Also, presumaing I'm unerstanding > > it's purpose correctly, ISTM it would fit along side other trace_* > > gucs in > > https://www.postgresql.org/docs/current/runtime-config-developer.html#RUNTIME-CONFIG-DEVELOPER. > > Not sure whether it's worth mentioning in release notes, but I think > you're right that it should be listed in that docs section. How about > the attached description? > Slightly modified version attached which I think is a little more succinct. > I see that there are two more developer-only GUCs that are not listed in > the docs: > > trace_syncscan > optimize_bounded_sort > > There's a comment on them that says "/* this is undocumented because not > exposed in a standard build */", but that seems like a weak reason, > given that many of the other options in that docs section also require > additional build-time options. I think we should add those to the docs > too for the sake of completeness. > Agreed. Robert Treat https://xzilla.net document-trace_connection_negotiation-2.patch Description: Binary data
Re: Comments on Custom RMGRs
On Tue, 2024-07-23 at 16:21 +0300, Heikki Linnakangas wrote: > So, I got a feeling that adding this to the rmgr interface is not > quite > right. The rmgr callbacks are for things that run when WAL is > *replayed*, while checkpoints are related to how WAL is generated. > Let's > design this as an independent hook, separate from rmgrs. That's a good way to look at it, agreed. Regards, Jeff Davis
Re: optimizing pg_upgrade's once-in-each-database steps
On 01.08.2024 22:41, Nathan Bossart wrote:
Here is a new patch set. Besides rebasing, I've added the recursive call
to process_slot() mentioned in the quoted text, and I've added quite a bit
of commentary to async.c.
That's much better now, thanks! Here's my code review, note that I
haven't tested the patches yet:
+void
+async_task_add_step(AsyncTask *task,
+ AsyncTaskGetQueryCB query_cb,
+ AsyncTaskProcessCB process_cb, bool free_result,
+ void *arg)
Is there any reason to have query as a callback function instead of char
*? From what I see right now, it doesn't give any extra flexibility, as
the query has to be static anyway (can't be customized on a per-database
basis) and will be created once before all the callbacks are run. While
passing in char * makes the API simpler, excludes any potential error of
making the query dependent on the current database and removes the
unnecessary malloc/free of the static strings.
+static void
+dispatch_query(const ClusterInfo *cluster, AsyncSlot *slot,
+ const AsyncTask *task)
+{
+ ...
+ if (!PQsendQuery(slot->conn, cbs->query))
+ conn_failure(slot->conn);
+}
This will print "connection failure: connection pointer is NULL", which
I don't think makes a lot of sense to the end user. I'd prefer something
like pg_fatal("failed to allocate a new connection").
if (found)
- pg_fatal("Data type checks failed: %s", report.data);
+ {
+ pg_fatal("Data type checks failed: %s",
data_type_check_report.data);
+ termPQExpBuffer(&data_type_check_report);
+ }
`found` should be removed and replaced with `data_type_check_failed`, as
it's not set anymore. Also the termPQExpBuffer after pg_fatal looks
unnecessary.
+static bool *data_type_check_results;
+static bool data_type_check_failed;
+static PQExpBufferData data_type_check_report;
IMO, it would be nicer to have these as a local state, that's passed in
as an arg* to the AsyncTaskProcessCB, which aligns with how the other
checks do it.
-- End of review --
Regarding keeping the connections, the way I envisioned it entailed
passing a list of connections from one check to the next one (or keeping
a global state with connections?). I didn't concretely look at the code
to verify this, so it's just an abstract idea.
Re: Thoughts about NUM_BUFFER_PARTITIONS
One of our customers recently asked me to look into buffer mapping. Following is my POV on the problem of optimal NUM_BUFFER_PARTITIONS. I’ve found some dead code: BufMappingPartitionLockByIndex() is unused, and unused for a long time. See patch 1. > On 23 Feb 2024, at 22:25, Tomas Vondra wrote: > > Well, if Postgres Pro implements this, I don't know what their reasoning > was exactly, but I guess they wanted to make it easier to experiment > with different values (without rebuild), or maybe they actually have > systems where they know higher values help ... > > Note: I'd point the maximum value 8 translates to 256, so no - it does > not max at the same value as PostgreSQL. I’ve prototyped similar GUC for anyone willing to do such experiments. See patch 2, 4. Probably, I’ll do some experiments too, on customer's clusters and workloads :) > Anyway, this value is inherently a trade off. If it wasn't, we'd set it > to something super high from the start. But having more partitions of > the lock table has a cost too, because some parts need to acquire all > the partition locks (and that's O(N) where N = number of partitions). I’ve found none such cases, actually. Or, perhaps, I was not looking good enough. pg_buffercache iterates over buffers and releases locks. See patch 3 to fix comments. > Of course, not having enough lock table partitions has a cost too, > because it increases the chance of conflict between backends (who happen > to need to operate on the same partition). This constant is not > constant, it changes over time - with 16 cores the collisions might have > been rare, with 128 not so much. Also, with partitioning we may need > many more locks per query. > > This means it's entirely possible it'd be good to have more than 128 > partitions of the lock table, but we only change this kind of stuff if > we have 2 things: > > 1) clear demonstration of the benefits (e.g. a workload showing an > improvement with higher number of partitions) > > 2) analysis of how this affects other workloads (e.g. cases that may > need to lock all the partitions etc) > > Ultimately it's a trade off - we need to judge if the impact in (2) is > worth the improvement in (1). > > None of this was done in this thread. There's no demonstration of the > benefits, no analysis of the impact etc. > > As for turning the parameter into a GUC, that has a cost too. Either > direct - a compiler can do far more optimizations with compile-time > constants than with values that may change during execution, for > example. I think overhead of finding partition by hash is negligible small. num_partitions in HTAB controls number of freelists. This might have some effect. > Or indirect - if we can't give users any guidance how/when to > tune the GUC, it can easily lead to misconfiguration (I can't even count > how many times I had to deal with systems where the values were "tuned" > following the logic that more is always better). Yes, this argument IMHO is most important. By doing more such knobs we promote superstitious approach to tuning. Best regards, Andrey Borodin. v0-0001-Remove-unused-functions-in-buf_internals.h.patch Description: Binary data v0-0002-GUCify-NUM_BUFFER_PARTITIONS.patch Description: Binary data v0-0003-Remove-reference-to-pg_buffercache-near-MAX_SIMUL.patch Description: Binary data v0-0004-Adjust-dshash.c-comment-about-NUM_BUFFER_PARTITIO.patch Description: Binary data
Re: psql: Add leakproof field to \dAo+ meta-command results
On 2024-07-30 08:30 +0200, Yugo NAGATA wrote: > On Tue, 30 Jul 2024 01:36:55 +0200 > Erik Wienhold wrote: > > > On 2024-07-01 15:08 +0200, Yugo NAGATA wrote: > > > I would like to propose to add a new field to psql's \dAo+ meta-command > > > to show whether the underlying function of an operator is leak-proof. > > > > +1 for making that info easily accessible. > > > > > This idea is inspired from [1] that claims some indexes uses non-LEAKPROOF > > > functions under the associated operators, as a result, it can not be > > > selected > > > for queries with security_barrier views or row-level security policies. > > > The original proposal was to add a query over system catalogs for looking > > > up > > > non-leakproof operators to the documentation, but I thought it is useful > > > to improve \dAo results rather than putting such query to the doc. > > > > > > The attached patch adds the field to \dAo+ and also a description that > > > explains the relation between indexes and security quals with referencing > > > \dAo+ meta-command. > > > > > > [1] > > > https://www.postgresql.org/message-id/raw/5af3bf0c-5e0c-4128-81dc-084c5258b1af%40code406.com > > > > \dAo+ output looks good. > > Thank you for looking into this. > I attached a patch updated with your suggestions. LGTM, thanks. > > > > But this patch fails regression tests in src/test/regress/sql/psql.sql > > (\dAo+ btree float_ops) because of the new leak-proof column. I think > > this could even be changed to "\dAo+ btree array_ops|float_ops" to also > > cover operators that are not leak-proof. > > Thank you for pointing out this. I fixed it with you suggestion to cover > non leak-proof operators, too. > > > + > > +For example, an index scan can not be selected for queries with > > > > I check the docs and "cannot" is more commonly used than "can not". > > Fixed. > > > > > +security_barrier views or row-level security > > policies if an > > +operator used in the WHERE clause is associated > > with the > > +operator family of the index, but its underlying function is not marked > > +LEAKPROOF. The program's > > +\dAo+ meta-command is useful for listing the > > operators > > +with associated operator families and whether it is leak-proof. > > + > > > > I think the last sentence can be improved. How about: "Use psql's \dAo+ > > command to list operator families and tell which of their operators are > > marked as leak-proof."? Should something similar be added to [1] which > > also talks about leak-proof operators? > > I agree, so I fixed the sentence as your suggestion and also add the > same description to the planner-stats-security doc. > > > The rest is just formatting nitpicks: > > > > + ", ofs.opfname AS \"%s\"\n," > > > > The trailing comma should come before the newline. > > > > + " CASE\n" > > + " WHEN p.proleakproof THEN '%s'\n" > > + " ELSE '%s'\n" > > + " END AS \"%s\"\n", > > > > WHEN/ELSE/END should be intended with one additional space to be > > consistent with the other CASE expressions in this query. > > Fixed both. > > Regards, > Yugo Nagata > > > > > [1] https://www.postgresql.org/docs/devel/planner-stats-security.html > > > > -- > > Erik > > > -- > Yugo NAGATA -- Erik
Re: Remove dependence on integer wrapping
On Fri, Jun 14, 2024 at 8:00 AM Alexander Lakhin wrote: > >And the most interesting case to me: >SET temp_buffers TO 10; > >CREATE TEMP TABLE t(i int PRIMARY KEY); >INSERT INTO t VALUES(1); > >#4 0x7f385cdd37f3 in __GI_abort () at ./stdlib/abort.c:79 >#5 0x5620071c4f51 in __addvsi3 () >#6 0x562007143f3c in init_htab (hashp=0x562008facb20, nelem=610070812) at dynahash.c:720 > >(gdb) f 6 >#6 0x560915207f3c in init_htab (hashp=0x560916039930, nelem=10) at dynahash.c:720 >720 hctl->high_mask = (nbuckets << 1) - 1; >(gdb) p nbuckets >$1 = 1073741824 Alex, are you able to get a full stack trace for this panic? I'm unable to reproduce this because I don't have enough memory in my system. I've tried reducing `BLCKSZ` to 1024, which is the lowest value allowed per my understanding, and I still don't have enough memory. Here's what it looks like is happening: 1. When inserting into the table, we create a new dynamic hash table and set `nelem` equal to `temp_buffers`, which is 10. 2. `nbuckets` is then set to the the next highest power of 2 from `nelem`, which is 1073741824. /* * Allocate space for the next greater power of two number of buckets, * assuming a desired maximum load factor of 1. */ nbuckets = next_pow2_int(nelem); 3. Shift `nbuckets` to the left by 1. This would equal 2147483648, which is larger than `INT_MAX`, which causes an overflow. hctl->high_mask = (nbuckets << 1) - 1; The max value allowed for `temp_buffers` is `INT_MAX / 2` (1073741823), So any value of `temp_buffers` in the range (536870912, 1073741823] would cause this overflow. Without `-ftrapv`, `nbuckets` would wrap around to -2147483648, which is likely to cause all sorts of havoc, I'm just not sure what exactly. Also, `nbuckets = next_pow2_int(nelem);`, by itself is a bit sketchy considering that `nelem` is a `long` and `nbuckets` is an `int`. Potentially, the fix here is to just convert `nbuckets` to a `long`. I plan on checking if that's feasible. I also found this commit [0] that increased the max of `nbuckets` from `INT_MAX / BLCKSZ` to `INT_MAX / 2`, which introduced the possibility of this overflow. So I plan on reading through that as well. Thanks, Joseph Koshakow [0] https://github.com/postgres/postgres/commit/0007490e0964d194a606ba79bb11ae1642da3372
Re: type cache cleanup improvements
Hi! On Wed, Apr 3, 2024 at 9:07 AM Andrei Lepikhov wrote: > On 3/15/24 17:57, Teodor Sigaev wrote: > >> Okay, I've applied this piece for now. Not sure I'll have much room > >> to look at the rest. > > > > Thank you very much! > I have spent some time reviewing this feature. I think we can discuss > and apply it step-by-step. So, the 0001-* patch is at this moment. > The feature addresses the issue of TypCache being bloated by intensive > usage of non-standard types and domains. It adds significant overhead > during relcache invalidation by thoroughly scanning this hash table. > IMO, this feature will be handy soon, as we already see some patches > where TypCache is intensively used for storing composite types—for > example, look into solutions proposed in [1]. > One of my main concerns with this feature is the possibility of lost > entries, which could be mistakenly used by relations with the same oid > in the future. This seems particularly possible in cases with multiple > temporary tables. The author has attempted to address this by replacing > the typrelid and type_id fields in the mapRelType on each call of > lookup_type_cache. However, I believe we could further improve this by > removing the entry from mapRelType on invalidation, thus avoiding this > potential issue. > While reviewing the patch, I made some minor changes (see attachment) > that you're free to adopt or reject. However, it's crucial that the > patch includes a detailed explanation, not just a single sentence, to > ensure everyone understands the changes. > Upon closer inspection, I noticed that the current implementation only > invalidates the cache entry. While this is acceptable for standard > types, it may not be sufficient to maintain numerous custom types (as in > the example in the initial letter) or in cases where whole-row vars are > heavily used. In such scenarios, removing the entry and reducing the > hash table's size might be more efficient. > In toto, the 0001-* patch looks good, and I would be glad to see it in > the core. I've revised the patchset. First of all, I've re-ordered the patches. 0001-0002 (former 0002-0003) Comprises hash_search_with_hash_value() function and its application to avoid full hash iteration in InvalidateAttoptCacheCallback() and TypeCacheTypCallback(). I think this is quite straightforward optimization without negative side effects. I've revised comments, commit message and did some code beautification. I'm going to push this if no objections. 0003 (former 0001) I've revised this patch. I think main concerns expressed in the thread about this path is that we don't have invalidation mechanism for relid => typid map. Finally due to oid wraparound same relids could get reused. That could lead to invalid entries in the map about existing relids and typeids. This is rather messy, but I don't think this could cause a material bug. The maps items are used only for cache invalidation. Extra invalidation doesn't cause a bug. If type with same relid will be cached, then correspoding map item will be overridden, so no missing invalidation. However, I see the following reasons for keeping consistent state of relid => typid map. 1) As the main use-case for this optimization is flood of temporary tables, it would be nice not let relid => typid map bloat in this case. I see that TypeCacheHash would get bloated, because its entries are never deleted. However, I would prefer to not get this situation even worse. 2) In future we may find some more use-cases for relid => typid map besides cache invalidation. Keeping that in consistent state could be advantage then. In the attached patch, I'm keeping relid => typid map when corresponding typentry have either TCFLAGS_HAVE_PG_TYPE_DATA, or TCFLAGS_OPERATOR_FLAGS, or tupdesc. Thus, when temporary table gets deleted, we would invalidate the map item. It will be also nice to get rid of iteration over all the cached domain types in TypeCacheRelCallback(). However, this typically shouldn't be a problem since domain types are less tended to bloat. Domain types are created manually, unlike composite types which are automatically created for every temporary table. We will probably need to optimize this in future, but I don't feel this to be necessary in present patch. I think the revised 0003 requires review. -- Regards, Alexander Korotkov Supabase v7-0001-Introduce-hash_search_with_hash_value-function.patch Description: Binary data v7-0002-Optimize-InvalidateAttoptCacheCallback-and-TypeCa.patch Description: Binary data v7-0003-Avoid-looping-over-all-type-cache-entries-in-Type.patch Description: Binary data
Fixed small typo in bufpage.h
Fixed a minor typo in src/include/storage/bufpage.h. Please let me know if any further action is required. --- Best regards, Senglee Choi v1-0001-fix-typo-in-bufpage-header-comment.patch Description: Binary data
Re: Fixed small typo in bufpage.h
Senglee Choi 于2024年8月5日周一 09:24写道： > Fixed a minor typo in src/include/storage/bufpage.h. > Please let me know if any further action is required. > Good catch. +1 -- Tender Wang
Re: Pgoutput not capturing the generated columns
Hi Shubhab.
Here are some more review comments for the v23-0001.
==
011_generated.pl b/src/test/subscription/t/011_generated.pl
nitpick - renamed /regress_pub/regress_pub_tab1/ and
/regress_sub1/regress_sub1_tab1/
nitpick - typo /inital data /initial data/
nitpick - typo /snode_subscriber2/node_subscriber2/
nitpick - tweak the combo initial sync comments and messages
nitpick - /#Cleanup/# cleanup/
nitpick - tweak all the combo normal replication comments
nitpick - removed blank line at the end
~~~
1. Refactor tab_gen_to_missing initial sync tests.
I moved the tab_gen_to_missing initial sync for node_subscriber2 to be
back where all the other initial sync tests are done.
See the nitpicks patch file.
~~~
2. Refactor tab_nogen_to_gen initial sync tests
I moved all the tab_nogen_to_gen initial sync tests back to where the
other initial sync tests are done.
See the nitpicks patch file.
~~~
3. Added another test case:
Because the (current PG17) nogen-to-gen initial sync test case (with
copy_data=true) gives an ERROR, I have added another combination to
cover normal replication (e.g. using copy_data=false).
See the nitpicks patch file.
(This has exposed an inconsistency which IMO might be a PG17 bug. I
have included TAP test comments about this, and plan to post a
separate thread for it later).
~
4. GUC
Moving and adding more CREATE SUBSCRIPTION exceeded some default GUCs,
so extra configuration was needed.
See the nitpick patch file.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/test/subscription/t/011_generated.pl
b/src/test/subscription/t/011_generated.pl
index 0b596b7..2be06c6 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -12,16 +12,25 @@ use Test::More;
my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+ "max_wal_senders = 20
+max_replication_slots = 20");
$node_publisher->start;
# All subscribers on this node will use parameter include_generated_columns =
false
my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
$node_subscriber->init;
+$node_subscriber->append_conf('postgresql.conf',
+ "max_logical_replication_workers = 20
+max_worker_processes = 20");
$node_subscriber->start;
# All subscribers on this node will use parameter include_generated_columns =
true
my $node_subscriber2 = PostgreSQL::Test::Cluster->new('subscriber2');
$node_subscriber2->init;
+$node_subscriber2->append_conf('postgresql.conf',
+ "max_logical_replication_workers = 20
+max_worker_processes = 20");
$node_subscriber2->start;
my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
@@ -139,7 +148,7 @@ $node_publisher->safe_psql('postgres',
# pub_combo_gen_to_missing is not included in pub_combo, because some tests
give errors.
$node_publisher->safe_psql('postgres',
- "CREATE PUBLICATION regress_pub FOR TABLE tab1");
+ "CREATE PUBLICATION regress_pub_tab1 FOR TABLE tab1");
$node_publisher->safe_psql('postgres',
"CREATE PUBLICATION regress_pub_combo FOR TABLE tab_gen_to_gen,
tab_gen_to_nogen, tab_missing_to_gen"
);
@@ -157,10 +166,10 @@ $node_publisher->safe_psql('postgres',
#
# Note that all subscriptions created on node_subscriber2 use copy_data =
false,
# because copy_data = true with include_generated_columns is not yet supported.
-# For this reason, the expected inital data on snode_subscriber2 is always
empty.
+# For this reason, the expected inital data on node_subscriber2 is always
empty.
$node_subscriber->safe_psql('postgres',
- "CREATE SUBSCRIPTION regress_sub1 CONNECTION '$publisher_connstr'
PUBLICATION regress_pub"
+ "CREATE SUBSCRIPTION regress_sub1_tab1 CONNECTION '$publisher_connstr'
PUBLICATION regress_pub_tab1"
);
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION regress_sub1_combo CONNECTION '$publisher_connstr'
PUBLICATION regress_pub_combo"
@@ -168,11 +177,18 @@ $node_subscriber->safe_psql('postgres',
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION regress_sub1_combo_gen_to_missing CONNECTION
'$publisher_connstr' PUBLICATION regress_pub_combo_gen_to_missing"
);
+# Note, regress_sub1_combo_nogen_to_gen is not created here due to expected
errors. See later.
$node_subscriber2->safe_psql('postgres',
"CREATE SUBSCRIPTION regress_sub2_combo CONNECTION '$publisher_connstr'
PUBLICATION regress_pub_combo WITH (include_generated_columns = true, copy_data
= false)"
);
$node_subscriber2->safe_psql('postgres',
+ "CREATE SUBSCRIPTION regress_sub2_combo_gen_to_missing CONNECTION
'$publisher_connstr' PUBLICATION regress_pub_combo_gen_to_missing with
(include_generated_columns = true, copy_data = false)"
+);
+$node_subscriber2->safe_psql('postgres',
+ "CREATE SUBSCRIPTION regress_sub2_combo_nogen_to_g
Re: Pgoutput not capturing the generated columns
Hi, Writing many new test case combinations has exposed a possible bug in patch 0001. In my previous post [1] there was questionable behaviour when replicating from a normal (not generated) column on the publisher side to a generated column on the subscriber side. Initially, I thought the test might have exposed a possible PG17 bug, but now I think it has really found a bug in patch 0001. ~~~ Previously (PG17) this would fail consistently both during COPY and during normal replication.Now, patch 0001 has changed this behaviour -- it is not always failing anymore. The patch should not be impacting this existing behaviour. It only introduces a new 'include_generated_columns', but since the publisher side is not a generated column I do not expect there should be any difference in behaviour for this test case. IMO the TAP test expected results should be corrected for this scenario. And fix the bug. Below is an example demonstrating PG17 behaviour. == Publisher: -- (notice column "b" is not generated) test_pub=# CREATE TABLE tab_nogen_to_gen (a int, b int); CREATE TABLE test_pub=# INSERT INTO tab_nogen_to_gen VALUES (1,101),(2,102); INSERT 0 2 test_pub=# CREATE PUBLICATION pub1 for TABLE tab_nogen_to_gen; CREATE PUBLICATION test_pub=# Subscriber: --- (notice corresponding column "b" is generated) test_sub=# CREATE TABLE tab_nogen_to_gen (a int, b int GENERATED ALWAYS AS (a * 22) STORED); CREATE TABLE test_sub=# Try to create a subscription. Notice we get the error: ERROR: logical replication target relation "public.tab_nogen_to_gen" is missing replicated column: "b" test_sub=# CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=test_pub' PUBLICATION pub1; 2024-08-05 13:16:40.043 AEST [20957] WARNING: subscriptions created by regression test cases should have names starting with "regress_" WARNING: subscriptions created by regression test cases should have names starting with "regress_" NOTICE: created replication slot "sub1" on publisher CREATE SUBSCRIPTION test_sub=# 2024-08-05 13:16:40.105 AEST [29258] LOG: logical replication apply worker for subscription "sub1" has started 2024-08-05 13:16:40.117 AEST [29260] LOG: logical replication table synchronization worker for subscription "sub1", table "tab_nogen_to_gen" has started 2024-08-05 13:16:40.172 AEST [29260] ERROR: logical replication target relation "public.tab_nogen_to_gen" is missing replicated column: "b" 2024-08-05 13:16:40.173 AEST [20039] LOG: background worker "logical replication tablesync worker" (PID 29260) exited with exit code 1 2024-08-05 13:16:45.187 AEST [29400] LOG: logical replication table synchronization worker for subscription "sub1", table "tab_nogen_to_gen" has started 2024-08-05 13:16:45.285 AEST [29400] ERROR: logical replication target relation "public.tab_nogen_to_gen" is missing replicated column: "b" 2024-08-05 13:16:45.286 AEST [20039] LOG: background worker "logical replication tablesync worker" (PID 29400) exited with exit code 1 ... Create the subscription again, but this time with copy_data = false test_sub=# CREATE SUBSCRIPTION sub1_nocopy CONNECTION 'dbname=test_pub' PUBLICATION pub1 WITH (copy_data = false); 2024-08-05 13:22:57.719 AEST [20957] WARNING: subscriptions created by regression test cases should have names starting with "regress_" WARNING: subscriptions created by regression test cases should have names starting with "regress_" NOTICE: created replication slot "sub1_nocopy" on publisher CREATE SUBSCRIPTION test_sub=# 2024-08-05 13:22:57.765 AEST [7012] LOG: logical replication apply worker for subscription "sub1_nocopy" has started test_sub=# ~~~ Then insert data from the publisher to see what happens for normal replication. test_pub=# test_pub=# INSERT INTO tab_nogen_to_gen VALUES (3,103),(4,104); INSERT 0 2 ~~~ Notice the subscriber gets the same error as before: ERROR: logical replication target relation "public.tab_nogen_to_gen" is missing replicated column: "b" 2024-08-05 13:25:14.897 AEST [20039] LOG: background worker "logical replication apply worker" (PID 10957) exited with exit code 1 2024-08-05 13:25:19.933 AEST [11095] LOG: logical replication apply worker for subscription "sub1_nocopy" has started 2024-08-05 13:25:19.966 AEST [11095] ERROR: logical replication target relation "public.tab_nogen_to_gen" is missing replicated column: "b" 2024-08-05 13:25:19.966 AEST [11095] CONTEXT: processing remote data for replication origin "pg_16390" during message type "INSERT" in transaction 742, finished at 0/1967BB0 2024-08-05 13:25:19.968 AEST [20039] LOG: background worker "logical replication apply worker" (PID 11095) exited with exit code 1 2024-08-05 13:25:24.917 AEST [11225] LOG: logical replication apply worker for subscription "sub1_nocopy" has started 2024-08-05 13:25:24.926 AEST [11225] ERROR: logical replication target relation "public.tab_nogen_to_gen" is missing replicated column: "b" 2024-08-05 13:25:24.926 AEST [11225] CONTEXT: proc
Re: Conflict detection and logging in logical replication
On Fri, Aug 2, 2024 at 6:28 PM Nisha Moond wrote: > > Performance tests done on the v8-0001 and v8-0002 patches, available at [1]. > Thanks for doing the detailed tests for this patch. > The purpose of the performance tests is to measure the impact on > logical replication with track_commit_timestamp enabled, as this > involves fetching the commit_ts data to determine > delete_differ/update_differ conflicts. > > Fortunately, we did not see any noticeable overhead from the new > commit_ts fetch and comparison logic. The only notable impact is > potential overhead from logging conflicts if they occur frequently. > Therefore, enabling conflict detection by default seems feasible, and > introducing a new detect_conflict option may not be necessary. > ... > > Test 1: create conflicts on Sub using pgbench. > > Setup: > - Both publisher and subscriber have pgbench tables created as- > pgbench -p $node1_port postgres -qis 1 > - At Sub, a subscription created for all the changes from Pub node. > > Test Run: > - To test, ran pgbench for 15 minutes on both nodes simultaneously, > which led to concurrent updates and update_differ conflicts on the > Subscriber node. > Command used to run pgbench on both nodes- > ./pgbench postgres -p 8833 -c 10 -j 3 -T 300 -P 20 > > Results: > For each case, note the “tps” and total time taken by the apply-worker > on Sub to apply the changes coming from Pub. > > Case1: track_commit_timestamp = off, detect_conflict = off > Pub-tps = 9139.556405 > Sub-tps = 8456.787967 > Time of replicating all the changes: 19min 28s > Case 2 : track_commit_timestamp = on, detect_conflict = on > Pub-tps = 8833.016548 > Sub-tps = 8389.763739 > Time of replicating all the changes: 20min 20s > Why is there a noticeable tps (~3%) reduction in publisher TPS? Is it the impact of track_commit_timestamp = on or something else? > Case3: track_commit_timestamp = on, detect_conflict = off > Pub-tps = 8886.101726 > Sub-tps = 8374.508017 > Time of replicating all the changes: 19min 35s > Case 4: track_commit_timestamp = off, detect_conflict = on > Pub-tps = 8981.924596 > Sub-tps = 8411.120808 > Time of replicating all the changes: 19min 27s > > **The difference of TPS between each case is small. While I can see a > slight increase of the replication time (about 5%), when enabling both > track_commit_timestamp and detect_conflict. > The difference in TPS between case 1 and case 2 is quite visible. IIUC, the replication time difference is due to the logging of conflicts, right? > Test2: create conflict using a manual script > > - To measure the precise time taken by the apply-worker in all cases, > create a test with a table having 10 million rows. > - To record the total time taken by the apply-worker, dump the > current time in the logfile for apply_handle_begin() and > apply_handle_commit(). > > Setup: > Pub : has a table ‘perf’ with 10 million rows. > Sub : has the same table ‘perf’ with its own 10 million rows (inserted > by 1000 different transactions). This table is subscribed for all > changes from Pub. > > Test Run: > At Pub: run UPDATE on the table ‘perf’ to update all its rows in a > single transaction. (this will lead to update_differ conflict for all > rows on Sub when enabled). > At Sub: record the time(from log file) taken by the apply-worker to > apply all updates coming from Pub. > > Results: > Below table shows the total time taken by the apply-worker > (apply_handle_commit Time - apply_handle_begin Time ). > (Two test runs for each of the four cases) > > Case1: track_commit_timestamp = off, detect_conflict = off > Run1 - 2min 42sec 579ms > Run2 - 2min 41sec 75ms > Case 2 : track_commit_timestamp = on, detect_conflict = on > Run1 - 6min 11sec 602ms > Run2 - 6min 25sec 179ms > Case3: track_commit_timestamp = on, detect_conflict = off > Run1 - 2min 34sec 223ms > Run2 - 2min 33sec 482ms > Case 4: track_commit_timestamp = off, detect_conflict = on > Run1 - 2min 35sec 276ms > Run2 - 2min 38sec 745ms > > ** In the case-2 when both track_commit_timestamp and detect_conflict > are enabled, the time taken by the apply-worker is ~140% higher. > > Test3: Case when no conflict is detected. > > To measure the time taken by the apply-worker when there is no > conflict detected. This test is to confirm if the time overhead in > Test1-Case2 is due to the new function GetTupleCommitTs() which > fetches the origin and timestamp information for each row in the table > before applying the update. > > Setup: > - The Publisher and Subscriber both have an empty table to start with. > - At Sub, the table is subscribed for all changes from Pub. > - At Pub: Insert 10 million rows and the same will be replicated to > the Sub ta
Re: pg_combinebackup does not detect missing files
On 8/2/24 20:37, Robert Haas wrote: On Fri, Apr 19, 2024 at 11:47 AM Robert Haas wrote: Hmm, that's an interesting perspective. I've always been very skeptical of doing verification only around missing files and not anything else. I figured that wouldn't be particularly meaningful, and that's pretty much the only kind of validation that's even theoretically possible without a bunch of extra overhead, since we compute checksums on entire files rather than, say, individual blocks. And you could really only do it for the final backup in the chain, because you should end up accessing all of those files, but the same is not true for the predecessor backups. So it's a very weak form of verification. But I looked into it and I think you're correct that, if you restrict the scope in the way that you suggest, we can do it without much additional code, or much additional run-time. The cost is basically that, instead of only looking for a backup_manifest entry when we think we can reuse its checksum, we need to do a lookup for every single file in the final input directory. Then, after processing all such files, we need to iterate over the hash table one more time and see what files were never touched. That seems like an acceptably low cost to me. So, here's a patch. I do think there's some chance that this will encourage people to believe that pg_combinebackup is better at finding problems than it really is or ever will be, and I also question whether it's right to keep changing stuff after feature freeze. But I have a feeling most people here are going to think this is worth including in 17. Let's see what others say. There was no hue and cry to include this in v17 and I think that ship has sailed at this point, but we could still choose to include this as an enhancement for v18 if people want it. I think David's probably in favor of that (but I'm not 100% sure) and I have mixed feelings about it (explained above) so what I'd really like is some other opinions on whether this idea is good, bad, or indifferent. I'm still in favor but if nobody else is interested then I'm not going to push on it. Regards, -David
Re: Conflict detection and logging in logical replication
On Sun, Aug 4, 2024 at 1:22 PM Zhijie Hou (Fujitsu) wrote: > > Here is the V11 patch set which addressed above and Kuroda-san[1] comments. > Thanks for the patch. Few comments: 1) Can you please recheck conflict.h inclusion. I think, these are not required: #include "access/xlogdefs.h" #include "executor/tuptable.h" #include "utils/relcache.h" Only these should suffice: #include "nodes/execnodes.h" #include "utils/timestamp.h" 2) create_subscription.sgml: For 'insert_exists' as well, we can mention that track_commit_timestamp should be enabled *on the susbcriber*. thanks Shveta
Re: Conflict detection and logging in logical replication
On Sun, Aug 4, 2024 at 1:04 PM Zhijie Hou (Fujitsu) wrote: > > On Friday, July 26, 2024 2:26 PM Amit Kapila wrote: > > > > > > I agree that displaying pk where applicable should be okay as we display it > > at > > other places but the same won't be possible when we do sequence scan to > > fetch the required tuple. So, the message will be different in that case, > > right? > > After some research, I think we can report the key values in DETAIL if the > apply worker uses any unique indexes to find the tuple to update/delete. > Otherwise, we can try to output all column values in DETAIL if the current > user > of apply worker has SELECT access to these columns. > I don't see any problem with displaying the column values in the LOG message when the user can access it. Also, we do the same in other places to further strengthen this idea. > This is consistent with what we do when reporting table constraint violation > (e.g. when violating a check constraint, it could output all the column value > if the current has access to all the column): > > - First, use super user to create a table. > CREATE TABLE t1 (c1 int, c2 int, c3 int check (c3 < 5)); > > - 1) using super user to insert a row that violates the constraint. We should > see all the column value. > > INSERT INTO t1(c3) VALUES (6); > ERROR: new row for relation "t1" violates check constraint > "t1_c3_check" > DETAIL: Failing row contains (null, null, 6). > > - 2) use a user without access to all the columns. We can only see the > inserted column and > CREATE USER regress_priv_user2; > GRANT INSERT (c1, c2, c3) ON t1 TO regress_priv_user2; > > SET SESSION AUTHORIZATION regress_priv_user2; > INSERT INTO t1 (c3) VALUES (6); > > ERROR: new row for relation "t1" violates check constraint > "t1_c3_check" > DETAIL: Failing row contains (c3) = (6). > > To achieve this, I think we can expose the ExecBuildSlotValueDescription > function and use it in conflict reporting. What do you think ? > Agreed. We should also consider displaying both the local and remote rows in case of update/delete_differ conflicts. Do, we have any case during conflict reporting where we won't have access to any of the columns? If so, we need to see what to display in such a case. -- With Regards, Amit Kapila.
Re: Conflict detection and logging in logical replication
On Mon, Aug 5, 2024 at 9:19 AM Amit Kapila wrote: > > On Fri, Aug 2, 2024 at 6:28 PM Nisha Moond wrote: > > > > Performance tests done on the v8-0001 and v8-0002 patches, available at [1]. > > > > Thanks for doing the detailed tests for this patch. > > > The purpose of the performance tests is to measure the impact on > > logical replication with track_commit_timestamp enabled, as this > > involves fetching the commit_ts data to determine > > delete_differ/update_differ conflicts. > > > > Fortunately, we did not see any noticeable overhead from the new > > commit_ts fetch and comparison logic. The only notable impact is > > potential overhead from logging conflicts if they occur frequently. > > Therefore, enabling conflict detection by default seems feasible, and > > introducing a new detect_conflict option may not be necessary. > > > ... > > > > Test 1: create conflicts on Sub using pgbench. > > > > Setup: > > - Both publisher and subscriber have pgbench tables created as- > > pgbench -p $node1_port postgres -qis 1 > > - At Sub, a subscription created for all the changes from Pub node. > > > > Test Run: > > - To test, ran pgbench for 15 minutes on both nodes simultaneously, > > which led to concurrent updates and update_differ conflicts on the > > Subscriber node. > > Command used to run pgbench on both nodes- > > ./pgbench postgres -p 8833 -c 10 -j 3 -T 300 -P 20 > > > > Results: > > For each case, note the “tps” and total time taken by the apply-worker > > on Sub to apply the changes coming from Pub. > > > > Case1: track_commit_timestamp = off, detect_conflict = off > > Pub-tps = 9139.556405 > > Sub-tps = 8456.787967 > > Time of replicating all the changes: 19min 28s > > Case 2 : track_commit_timestamp = on, detect_conflict = on > > Pub-tps = 8833.016548 > > Sub-tps = 8389.763739 > > Time of replicating all the changes: 20min 20s > > > > Why is there a noticeable tps (~3%) reduction in publisher TPS? Is it > the impact of track_commit_timestamp = on or something else? Was track_commit_timestamp enabled only on subscriber (as needed) or on both publisher and subscriber? Nisha, can you please confirm from your logs? > > Case3: track_commit_timestamp = on, detect_conflict = off > > Pub-tps = 8886.101726 > > Sub-tps = 8374.508017 > > Time of replicating all the changes: 19min 35s > > Case 4: track_commit_timestamp = off, detect_conflict = on > > Pub-tps = 8981.924596 > > Sub-tps = 8411.120808 > > Time of replicating all the changes: 19min 27s > > > > **The difference of TPS between each case is small. While I can see a > > slight increase of the replication time (about 5%), when enabling both > > track_commit_timestamp and detect_conflict. > > > > The difference in TPS between case 1 and case 2 is quite visible. > IIUC, the replication time difference is due to the logging of > conflicts, right? > > > Test2: create conflict using a manual script > > > > - To measure the precise time taken by the apply-worker in all cases, > > create a test with a table having 10 million rows. > > - To record the total time taken by the apply-worker, dump the > > current time in the logfile for apply_handle_begin() and > > apply_handle_commit(). > > > > Setup: > > Pub : has a table ‘perf’ with 10 million rows. > > Sub : has the same table ‘perf’ with its own 10 million rows (inserted > > by 1000 different transactions). This table is subscribed for all > > changes from Pub. > > > > Test Run: > > At Pub: run UPDATE on the table ‘perf’ to update all its rows in a > > single transaction. (this will lead to update_differ conflict for all > > rows on Sub when enabled). > > At Sub: record the time(from log file) taken by the apply-worker to > > apply all updates coming from Pub. > > > > Results: > > Below table shows the total time taken by the apply-worker > > (apply_handle_commit Time - apply_handle_begin Time ). > > (Two test runs for each of the four cases) > > > > Case1: track_commit_timestamp = off, detect_conflict = off > > Run1 - 2min 42sec 579ms > > Run2 - 2min 41sec 75ms > > Case 2 : track_commit_timestamp = on, detect_conflict = on > > Run1 - 6min 11sec 602ms > > Run2 - 6min 25sec 179ms > > Case3: track_commit_timestamp = on, detect_conflict = off > > Run1 - 2min 34sec 223ms > > Run2 - 2min 33sec 482ms > > Case 4: track_commit_timestamp = off, detect_conflict = on > > Run1 - 2min 35sec 276ms > > Run2 - 2min 38sec 745ms > > > > ** In the case-2 when both track_commit_timestamp and detect_conflict > > are enabled, the time taken by the apply-worker is ~140% higher. > > > > Test3: Case when no conflict is detected. > > > > To measure the time taken by the apply-worker when there is no > > conflict detected. This test is to confir
Re: Logical Replication of sequences
On Thu, 1 Aug 2024 at 03:33, Peter Smith wrote:
>
> Hi Vignesh,
>
> I have a question about the subscriber-side behaviour of currval().
>
> ==
>
> AFAIK it is normal for currval() to give error is nextval() has not
> yet been called [1]
>
> For example.
> test_pub=# create sequence s1;
> CREATE SEQUENCE
> test_pub=# select * from currval('s1');
> 2024-08-01 07:42:48.619 AEST [24131] ERROR: currval of sequence "s1"
> is not yet defined in this session
> 2024-08-01 07:42:48.619 AEST [24131] STATEMENT: select * from currval('s1');
> ERROR: currval of sequence "s1" is not yet defined in this session
> test_pub=# select * from nextval('s1');
> nextval
> -
>1
> (1 row)
>
> test_pub=# select * from currval('s1');
> currval
> -
>1
> (1 row)
>
> test_pub=#
>
> ~~~
>
> OTOH, I was hoping to be able to use currval() at the subscriber=side
> to see the current sequence value after issuing ALTER .. REFRESH
> PUBLICATION SEQUENCES.
>
> Unfortunately, it has the same behaviour where currval() cannot be
> used without nextval(). But, on the subscriber, you probably never
> want to do an explicit nextval() independently of the publisher.
>
> Is this currently a bug, or maybe a quirk that should be documented?
The currval returns the most recent value obtained from the nextval
function for a given sequence within the current session. This
function is specific to the session, meaning it only provides the last
sequence value retrieved during that session. However, if you call
currval before using nextval in the same session, you'll encounter an
error stating "currval of the sequence is not yet defined in this
session." Meaning even in the publisher this value is only visible in
the current session and not in a different session. Alternatively you
can use the following to get the last_value of the sequence: SELECT
last_value FROM sequence_name. I feel this need not be documented as
the similar issue is present in the publisher and there is an "SELECT
last_value FROM sequence_name" to get the last_value.
Regards,
Vignesh
Re: Logical Replication of sequences
On Thu, 1 Aug 2024 at 04:25, Peter Smith wrote:
>
> Hi Vignesh,
>
> I noticed that when replicating sequences (using the latest patches
> 0730_2*) the subscriber-side checks the *existence* of the sequence,
> but apparently it is not checking other sequence attributes.
>
> For example, consider:
>
> Publisher: "CREATE SEQUENCE s1 START 1 INCREMENT 2;" should be a
> sequence of only odd numbers.
> Subscriber: "CREATE SEQUENCE s1 START 2 INCREMENT 2;" should be a
> sequence of only even numbers.
>
> Because the names match, currently the patch allows replication of the
> s1 sequence. I think that might lead to unexpected results on the
> subscriber. IMO it might be safer to report ERROR unless the sequences
> match properly (i.e. not just a name check).
>
> Below is a demonstration the problem:
>
> ==
> Publisher:
> ==
>
> (publisher sequence is odd numbers)
>
> test_pub=# create sequence s1 start 1 increment 2;
> CREATE SEQUENCE
> test_pub=# select * from nextval('s1');
> nextval
> -
>1
> (1 row)
>
> test_pub=# select * from nextval('s1');
> nextval
> -
>3
> (1 row)
>
> test_pub=# select * from nextval('s1');
> nextval
> -
>5
> (1 row)
>
> test_pub=# CREATE PUBLICATION pub1 FOR ALL SEQUENCES;
> CREATE PUBLICATION
> test_pub=#
>
> ==
> Subscriber:
> ==
>
> (subscriber sequence is even numbers)
>
> test_sub=# create sequence s1 start 2 increment 2;
> CREATE SEQUENCE
> test_sub=# SELECT * FROM nextval('s1');
> nextval
> -
>2
> (1 row)
>
> test_sub=# SELECT * FROM nextval('s1');
> nextval
> -
>4
> (1 row)
>
> test_sub=# SELECT * FROM nextval('s1');
> nextval
> -
>6
> (1 row)
>
> test_sub=# CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=test_pub'
> PUBLICATION pub1;
> 2024-08-01 08:43:04.198 AEST [24325] WARNING: subscriptions created
> by regression test cases should have names starting with "regress_"
> WARNING: subscriptions created by regression test cases should have
> names starting with "regress_"
> NOTICE: created replication slot "sub1" on publisher
> CREATE SUBSCRIPTION
> test_sub=# 2024-08-01 08:43:04.294 AEST [26240] LOG: logical
> replication apply worker for subscription "sub1" has started
> 2024-08-01 08:43:04.309 AEST [26244] LOG: logical replication
> sequence synchronization worker for subscription "sub1" has started
> 2024-08-01 08:43:04.323 AEST [26244] LOG: logical replication
> synchronization for subscription "sub1", sequence "s1" has finished
> 2024-08-01 08:43:04.323 AEST [26244] LOG: logical replication
> sequence synchronization worker for subscription "sub1" has finished
>
> (after the CREATE SUBSCRIPTION we are getting replicated odd values
> from the publisher, even though the subscriber side sequence was
> supposed to be even numbers)
>
> test_sub=# SELECT * FROM nextval('s1');
> nextval
> -
>7
> (1 row)
>
> test_sub=# SELECT * FROM nextval('s1');
> nextval
> -
>9
> (1 row)
>
> test_sub=# SELECT * FROM nextval('s1');
> nextval
> -
> 11
> (1 row)
>
> (Looking at the description you would expect odd values for this
> sequence to be impossible)
>
> test_sub=# \dS+ s1
> Sequence "public.s1"
> Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
> +---+-+-+---+-+---
> bigint | 2 | 1 | 9223372036854775807 | 2 | no | 1
Even if we check the sequence definition during the CREATE
SUBSCRIPTION/ALTER SUBSCRIPTION ... REFRESH PUBLICATION or ALTER
SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES commands, there's still
a chance that the sequence definition might change after the command
has been executed. Currently, there's no mechanism to lock a sequence,
and we also permit replication of table data even if the table
structures differ, such as mismatched data types like int and
smallint. I have modified it to log a warning to inform users that the
sequence options on the publisher and subscriber are not the same and
advise them to ensure that the sequence definitions are consistent
between both.
The v20240805 version patch attached at [1] has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CALDaNm1Y_ot-jFRfmtwDuwmFrgSSYHjVuy28RspSopTtwzXy8w%40mail.gmail.com
Regards,
Vignesh
Re: Conflict detection and logging in logical replication
On Mon, Aug 5, 2024 at 10:05 AM shveta malik wrote: > > On Mon, Aug 5, 2024 at 9:19 AM Amit Kapila wrote: > > > > On Fri, Aug 2, 2024 at 6:28 PM Nisha Moond wrote: > > > > > > Test Summary - > > > -- The duration for case-2 was reduced to 2-3 minutes, matching the > > > times of the other cases. > > > -- The test revealed that the overhead in case-2 was not due to > > > commit_ts fetching (GetTupleCommitTs). > > > -- The additional action in case-2 was the error logging of all 10 > > > million update_differ conflicts. > > > > > > > According to me, this last point is key among all tests which will > > decide whether we should have a new subscription option like > > detect_conflict or not. I feel this is the worst case where all the > > row updates have conflicts and the majority of time is spent writing > > LOG messages. Now, for this specific case, if one wouldn't have > > enabled track_commit_timestamp then there would be no difference as > > seen in case-4. So, I don't see this as a reason to introduce a new > > subscription option like detect_conflicts, if one wants to avoid such > > an overhead, she shouldn't have enabled track_commit_timestamp in the > > first place to detect conflicts. Also, even without this, we would see > > similar overhead in the case of update/delete_missing where we LOG > > when the tuple to modify is not found. > > > > Overall, it looks okay to get rid of the 'detect_conflict' parameter. > My only concern here is the purpose/use-cases of > 'track_commit_timestamp'. Is the only purpose of enabling > 'track_commit_timestamp' is to detect conflicts? I couldn't find much > in the doc on this. Can there be a case where a user wants to enable > 'track_commit_timestamp' for any other purpose without enabling > subscription's conflict detection? > I am not aware of any other use case for 'track_commit_timestamp' GUC. As per my understanding, commit timestamp is primarily required for conflict detection and resolution. We can probably add a description in 'track_commit_timestamp' GUC about its usage along with this patch. -- With Regards, Amit Kapila.
Re: relfilenode statistics
Hi,
On Thu, Jul 11, 2024 at 06:10:23AM +, Bertrand Drouvot wrote:
> Hi,
>
> On Thu, Jul 11, 2024 at 01:58:19PM +0900, Michael Paquier wrote:
> > On Wed, Jul 10, 2024 at 01:38:06PM +, Bertrand Drouvot wrote:
> > > So, I think it makes sense to link the hashkey to all the RelFileLocator
> > > fields, means:
> > >
> > > dboid (linked to RelFileLocator's dbOid)
> > > objoid (linked to RelFileLocator's spcOid)
> > > relfile (linked to RelFileLocator's relNumber)
> >
> > Hmm. How about using the table OID as objoid,
>
> The issue is that we don't have the relation OID when writing buffers out
> (that's
> one of the reason explained in [1]).
>
> [1]:
> https://www.postgresql.org/message-id/Zl2k8u4HDTUW6QlC%40ip-10-97-1-34.eu-west-3.compute.internal
>
> Regards,
>
Please find attached a mandatory rebase due to the recent changes around
statistics.
As mentioned up-thread:
The attached patch is not in a fully "polished" state yet: there is more places
we should add relfilenode counters, create more APIS to retrieve the relfilenode
stats
It is in a state that can be used to discuss the approach it is implementing (as
we have done so far) before moving forward.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From 1df7f2eed01478cdbe36673ef18247452e579f3b Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot
Date: Thu, 16 Nov 2023 02:30:01 +
Subject: [PATCH v3] Provide relfilenode statistics
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
We currently don’t have writes counters for relations.
The reason is that we don’t have the relation OID when writing buffers out.
Tracking writes per relfilenode would allow us to track/consolidate writes per
relation.
relfilenode stats is also beneficial for the "Split index and table statistics
into different types of stats" work in progress: it would allow us to avoid
additional branches in some situations.
=== Remarks ===
This is a POC patch. There is still work to do: there is more places we should
add relfilenode counters, create more APIS to retrieve the relfilenode stats,
the patch takes care of rewrite generated by TRUNCATE but there is more to
care about like CLUSTER,VACUUM FULL.
The new logic to retrieve stats in pg_statio_all_tables has been implemented
only for the new blocks_written stat (we'd need to do the same for the existing
buffer read / buffer hit if we agree on the approach implemented here).
The goal of this patch is to start the discussion and agree on the design before
moving forward.
---
src/backend/access/rmgrdesc/xactdesc.c| 5 +-
src/backend/catalog/storage.c | 8 ++
src/backend/catalog/system_functions.sql | 2 +-
src/backend/catalog/system_views.sql | 5 +-
src/backend/postmaster/checkpointer.c | 5 +
src/backend/storage/buffer/bufmgr.c | 6 +-
src/backend/storage/smgr/md.c | 7 ++
src/backend/utils/activity/pgstat.c | 39 --
src/backend/utils/activity/pgstat_database.c | 12 +-
src/backend/utils/activity/pgstat_function.c | 13 +-
src/backend/utils/activity/pgstat_relation.c | 112 --
src/backend/utils/activity/pgstat_replslot.c | 13 +-
src/backend/utils/activity/pgstat_shmem.c | 19 ++-
.../utils/activity/pgstat_subscription.c | 12 +-
src/backend/utils/activity/pgstat_xact.c | 60 +++---
src/backend/utils/adt/pgstatfuncs.c | 34 +-
src/include/access/tableam.h | 19 +++
src/include/access/xact.h | 1 +
src/include/catalog/pg_proc.dat | 14 ++-
src/include/pgstat.h | 37 --
src/include/utils/pgstat_internal.h | 34 --
src/test/recovery/t/029_stats_restart.pl | 40 +++
.../recovery/t/030_stats_cleanup_replica.pl | 6 +-
src/test/regress/expected/rules.out | 12 +-
src/test/regress/expected/stats.out | 30 ++---
src/test/regress/sql/stats.sql| 30 ++---
src/test/subscription/t/026_stats.pl | 4 +-
src/tools/pgindent/typedefs.list | 1 +
28 files changed, 424 insertions(+), 156 deletions(-)
4.4% src/backend/catalog/
46.2% src/backend/utils/activity/
6.3% src/backend/utils/adt/
3.6% src/backend/
3.2% src/include/access/
3.2% src/include/catalog/
6.0% src/include/utils/
6.6% src/include/
11.7% src/test/recovery/t/
5.3% src/test/regress/expected/
3.0% src/
diff --git a/src/backend/access/rmgrdesc/xactdesc.c b/src/backend/access/rmgrdesc/xactdesc.c
index dccca201e0..c02b079645 100644
--- a/src/backend/access/rmgrdesc/xactdesc.c
+++ b/src/backend/access/rmgrdesc/xactdesc.c
@@ -319,10 +319,11 @@ xact_desc_stats(StringInfo buf, const char *label,
appendStringInfo(buf, "; %sdropped stats:", label);
for (i = 0; i < ndropped; i++)
{
-
Re: Logical Replication of sequences
On Wed, 31 Jul 2024 at 14:39, shveta malik wrote:
>
> On Mon, Jun 10, 2024 at 5:00 PM vignesh C wrote:
> >
> > On Mon, 10 Jun 2024 at 12:24, Amul Sul wrote:
> > >
> > >
> > >
> > > On Sat, Jun 8, 2024 at 6:43 PM vignesh C wrote:
> > >>
> > >> On Wed, 5 Jun 2024 at 14:11, Amit Kapila wrote:
> > >> [...]
> > >> A new catalog table, pg_subscription_seq, has been introduced for
> > >> mapping subscriptions to sequences. Additionally, the sequence LSN
> > >> (Log Sequence Number) is stored, facilitating determination of
> > >> sequence changes occurring before or after the returned sequence
> > >> state.
> > >
> > >
> > > Can't it be done using pg_depend? It seems a bit excessive unless I'm
> > > missing
> > > something.
> >
> > We'll require the lsn because the sequence LSN informs the user that
> > it has been synchronized up to the LSN in pg_subscription_seq. Since
> > we are not supporting incremental sync, the user will be able to
> > identify if he should run refresh sequences or not by checking the lsn
> > of the pg_subscription_seq and the lsn of the sequence(using
> > pg_sequence_state added) in the publisher.
>
> How the user will know from seq's lsn that he needs to run refresh.
> lsn indicates page_lsn and thus the sequence might advance on pub
> without changing lsn and thus lsn may look the same on subscriber even
> though a sequence-refresh is needed. Am I missing something here?
When a sequence is synchronized to the subscriber, the page LSN of the
sequence from the publisher is also retrieved and stored in
pg_subscriber_rel as shown below:
--- Publisher page lsn
publisher=# select pg_sequence_state('seq1');
pg_sequence_state
(0/1510E38,65,1,t)
(1 row)
--- Subscriber stores the publisher's page lsn for the sequence
subscriber=# select * from pg_subscription_rel where srrelid = 16384;
srsubid | srrelid | srsubstate | srsublsn
-+-++---
16389 | 16384 | r | 0/1510E38
(1 row)
If changes are made to the sequence, such as performing many nextvals,
the page LSN will be updated. Currently the sequence values are
prefetched for SEQ_LOG_VALS 32, so the lsn will not get updated for
the prefetched values, once the prefetched values are consumed the lsn
will get updated.
For example:
--- Updated LSN on the publisher (old lsn - 0/1510E38, new lsn - 0/1558CA8)
publisher=# select pg_sequence_state('seq1');
pg_sequence_state
--
(0/1558CA8,143,22,t)
(1 row)
The user can then compare this updated value with the sequence's LSN
in pg_subscription_rel to determine when to re-synchronize the
sequence.
Regards,
Vignesh
Re: consider -Wmissing-variable-declarations
On 03.08.24 22:46, Tom Lane wrote: Peter Eisentraut writes: This has all been committed now. Various buildfarm animals are complaining about g++ -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Werror=unguarded-availability-new -Wendif-labels -Wmissing-format-attribute -Wcast-function-type -Wformat-security -Wmissing-variable-declarations -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-unused-command-line-argument -Wno-compound-token-split-by-macro -Wno-cast-function-type-strict -g -O2 -Wno-deprecated-declarations -fPIC -fvisibility=hidden -shared -o llvmjit.so llvmjit.o llvmjit_error.o llvmjit_inline.o llvmjit_wrap.o llvmjit_deform.o llvmjit_expr.o -L../../../../src/port -L../../../../src/common -L/usr/lib64 -Wl,--as-needed -Wl,-rpath,'/home/centos/17-lancehead/buildroot/HEAD/inst/lib',--enable-new-dtags -fvisibility=hidden -lLLVM-17 g++: error: unrecognized command line option \342\200\230-Wmissing-variable-declarations\342\200\231; did you mean \342\200\230-Wmissing-declarations\342\200\231? make[2]: *** [../../../../src/Makefile.shlib:261: llvmjit.so] Error 1 It looks like we are passing CFLAGS not CXXFLAGS to this particular g++ invocation. Changing this seems to have done the trick.
Remove support for old realpath() API
The now preferred way to call realpath() is by passing NULL as the
second argument and get a malloc'ed result. We still supported the
old way of providing our own buffer as a second argument, for some
platforms that didn't support the new way yet. Those were only
Solaris less than version 11 and some older AIX versions (7.1 and
newer appear to support the new variant). We don't support those
platforms versions anymore, so we can remove this extra code.From aba5b5b6017eff0baa59698de665058843fe1d05 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 5 Aug 2024 07:50:27 +0200
Subject: [PATCH] Remove support for old realpath() API
The now preferred way to call realpath() is by passing NULL as the
second argument and get a malloc'ed result. We still supported the
old way of providing our own buffer as a second argument, for some
platforms that didn't support the new way yet. Those were only
Solaris less than version 11 and some older AIX versions (7.1 and
newer appear to support the new variant). We don't support those
platforms versions anymore, so we can remove this extra code.
---
src/common/exec.c | 19 ---
1 file changed, 19 deletions(-)
diff --git a/src/common/exec.c b/src/common/exec.c
index 0bee19c1e53..32fd56532aa 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -285,25 +285,6 @@ pg_realpath(const char *fname)
#ifndef WIN32
path = realpath(fname, NULL);
- if (path == NULL && errno == EINVAL)
- {
- /*
-* Cope with old-POSIX systems that require a user-provided
buffer.
-* Assume MAXPGPATH is enough room on all such systems.
-*/
- char *buf = malloc(MAXPGPATH);
-
- if (buf == NULL)
- return NULL;/* assume errno is set */
- path = realpath(fname, buf);
- if (path == NULL) /* don't leak memory */
- {
- int save_errno = errno;
-
- free(buf);
- errno = save_errno;
- }
- }
#else /* WIN32 */
/*
--
2.46.0
Re: Pluggable cumulative statistics
On Fri, Aug 02, 2024 at 05:53:31AM +0900, Michael Paquier wrote: > Attached is a rebased set of the rest, with 0001 now introducing the > pluggable core part. So, I have been able to spend a few more days on all that while travelling across three continents, and I have applied the core patch followed by the template parts after more polishing. The core part has been tweaked a bit more in terms of variable and structure names, to bring the builtin and custom stats parts more consistent with each other. There were also a bunch of loops that did not use the PgStat_Kind, but an int with an index on the custom_data arrays. I have uniformized the whole. I am keeping an eye on the buildfarm and it is currently green. My machines don't seem to have run the new tests with injection points yet, the CI on the CF app is not reporting any failure caused by that, and my CI runs have all been stable. -- Michael signature.asc Description: PGP signature
