On Wed, Apr 13, 2022 at 10:01 PM Alvaro Herrera wrote:
>
> BTW I just noticed that AlterPublicationOptions acquires only
> ShareAccessLock on the publication object. I think this is too lax ...
> what if two of them run concurrently? (say to specify different
> published actions) Do they
On 2022-Apr-13, Amit Kapila wrote:
> Thanks, this will work and fix the issue. I think this looks better
> than the current code,
Thanks for looking! Pushed.
> however, I am not sure if the handling for the
> concurrently dropped tables is better (both get_rel_relkind() and
> get_rel_name()
On Tue, Apr 12, 2022 at 6:16 PM Alvaro Herrera wrote:
>
> On 2022-Apr-12, Amit Kapila wrote:
>
> > It still has the same problem. The table can be dropped just before
> > this message and the get_rel_name will return NULL and we don't expect
> > that.
>
> Ugh, I forgot to change the errmsg()
On 2022-Apr-12, Amit Kapila wrote:
> I mean that it fetches the tuple from the RELOID cache and then
> performs relkind and other checks similar to what we are doing. I
> think it could also have used get_rel_relkind() but probably not done
> because it doesn't have a lock on the relation.
Ah,
On 2022-Apr-12, Amit Kapila wrote:
> It still has the same problem. The table can be dropped just before
> this message and the get_rel_name will return NULL and we don't expect
> that.
Ugh, I forgot to change the errmsg() parts to use the new variable,
apologies. Fixed.
> Also, is there a
On Tue, Apr 12, 2022 at 5:01 PM Alvaro Herrera wrote:
>
> On 2022-Apr-12, Amit Kapila wrote:
>
> > On Tue, Apr 12, 2022 at 3:45 PM Amit Kapila wrote:
>
> > > We don't have a lock on the relation, so if it gets dropped
> > > concurrently, it won't behave sanely. For example, get_rel_name() will
>
On Tue, Apr 12, 2022 at 5:12 PM Alvaro Herrera wrote:
>
> Sorry, I think I neglected to "git add" some late changes.
>
+ if (has_rowfilter)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot set parameter \"%s\" to false for publication \"%s\"",
+
Sorry, I think I neglected to "git add" some late changes.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
>From e7569ed4c4a01f782f9326ebc9a3c9049973ef4b Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 12 Apr 2022 12:59:50 +0200
Subject: [PATCH v3]
On 2022-Apr-12, Amit Kapila wrote:
> On Tue, Apr 12, 2022 at 3:45 PM Amit Kapila wrote:
> > We don't have a lock on the relation, so if it gets dropped
> > concurrently, it won't behave sanely. For example, get_rel_name() will
> > return NULL which seems incorrect to me.
Oh, oops ... a trap
On Tue, Apr 12, 2022 at 3:45 PM Amit Kapila wrote:
>
> On Tue, Apr 12, 2022 at 2:35 PM Alvaro Herrera
> wrote:
> >
> > I understand that this is a minimal fix, and for that it seems OK, but I
> > think the surrounding style is rather baroque. This code can be made
> > simpler. Here's my take
On Tue, Apr 12, 2022 at 2:35 PM Alvaro Herrera wrote:
>
> I understand that this is a minimal fix, and for that it seems OK, but I
> think the surrounding style is rather baroque. This code can be made
> simpler. Here's my take on it.
>
We don't have a lock on the relation, so if it gets
I understand that this is a minimal fix, and for that it seems OK, but I
think the surrounding style is rather baroque. This code can be made
simpler. Here's my take on it. I think it's also faster: we avoid
looking up pg_publication_rel entries for rels that aren't partitioned
tables.
--
On Tue, Apr 12, 2022 at 11:31 AM houzj.f...@fujitsu.com
wrote:
>
> On Tuesday, April 12, 2022 8:40 AM Peter Smith wrote:
> >
> > FYI, I was playing with row filters and partitions recently, and while doing
> > something a bit unusual I received a cache leak warning.
> >
> > Below are the steps
On Tuesday, April 12, 2022 8:40 AM Peter Smith wrote:
>
> FYI, I was playing with row filters and partitions recently, and while doing
> something a bit unusual I received a cache leak warning.
>
> Below are the steps to reproduce it:
>
>
> test_pub=# CREATE TABLE parent(a int primary key)
FYI, I was playing with row filters and partitions recently, and while
doing something a bit unusual I received a cache leak warning.
Below are the steps to reproduce it:
test_pub=# CREATE TABLE parent(a int primary key) PARTITION BY RANGE(a);
CREATE TABLE
test_pub=# CREATE TABLE child
On Mon, Mar 7, 2022 at 12:50 AM Tomas Vondra
wrote:
>
> On 3/3/22 21:07, Euler Taveira wrote:
> > On Thu, Mar 3, 2022, at 7:47 AM, Amit Kapila wrote:
> >> LGTM. I'll push this tomorrow unless Tomas or Euler feels otherwise.
> > Sounds good to me.
> >
>
> +1
>
Thanks, Pushed
On 3/3/22 21:07, Euler Taveira wrote:
> On Thu, Mar 3, 2022, at 7:47 AM, Amit Kapila wrote:
>> LGTM. I'll push this tomorrow unless Tomas or Euler feels otherwise.
> Sounds good to me.
>
+1
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Mar 3, 2022, at 7:47 AM, Amit Kapila wrote:
> LGTM. I'll push this tomorrow unless Tomas or Euler feels otherwise.
Sounds good to me.
--
Euler Taveira
EDB https://www.enterprisedb.com/
On Thu, Mar 3, 2022 at 11:18 AM shiy.f...@fujitsu.com
wrote:
>
> On Thu, Mar 3, 2022 10:40 AM Amit Kapila wrote:
> >
> > On Wed, Mar 2, 2022 at 5:42 PM Euler Taveira wrote:
> > >
> > > On Wed, Mar 2, 2022, at 8:45 AM, Tomas Vondra wrote:
> > >
> > > While working on the column filtering patch,
On Thu, Mar 3, 2022 10:40 AM Amit Kapila wrote:
>
> On Wed, Mar 2, 2022 at 5:42 PM Euler Taveira wrote:
> >
> > On Wed, Mar 2, 2022, at 8:45 AM, Tomas Vondra wrote:
> >
> > While working on the column filtering patch, which touches about the
> > same places, I noticed two minor gaps in testing:
On Wed, Mar 2, 2022 at 5:42 PM Euler Taveira wrote:
>
> On Wed, Mar 2, 2022, at 8:45 AM, Tomas Vondra wrote:
>
> While working on the column filtering patch, which touches about the
> same places, I noticed two minor gaps in testing:
>
> 1) The regression tests do perform multiple ALTER
On Wed, Mar 2, 2022, at 8:45 AM, Tomas Vondra wrote:
> While working on the column filtering patch, which touches about the
> same places, I noticed two minor gaps in testing:
>
> 1) The regression tests do perform multiple ALTER PUBLICATION commands,
> tweaking the row filter. But there are no
Hi,
While working on the column filtering patch, which touches about the
same places, I noticed two minor gaps in testing:
1) The regression tests do perform multiple ALTER PUBLICATION commands,
tweaking the row filter. But there are no checks the row filter was
actually modified / stored in the
On Thu, Feb 24, 2022 at 1:33 PM Amit Kapila wrote:
>
> On Thu, Feb 24, 2022 at 7:57 AM Peter Smith wrote:
> >
> > I noticed that there was a build-farm failure on the machine 'komodoensis'
> > [1]
> >
> > # Failed test 'check replicated rows to tab_rowfilter_toast'
> > # at
On Thu, Feb 24, 2022 at 7:57 AM Peter Smith wrote:
>
> I noticed that there was a build-farm failure on the machine 'komodoensis' [1]
>
> # Failed test 'check replicated rows to tab_rowfilter_toast'
> # at t/028_row_filter.pl line 687.
> # got: ''
> # expected: 't|1'
> # Looks
j.f...@fujitsu.com; Peter Smith
; Alvaro Herrera ; Greg
Nancarrow ; vignesh C ; Ajin Cherian
; tanghy.f...@fujitsu.com; Dilip Kumar
; Rahila Syed ; Peter Eisentraut
; Önder Kalacı ;
japin ; Michael Paquier ; David
Steele ; Craig Ringer ; Amit
Langote ; PostgreSQL Hackers
Subject: Re: row filter
I noticed that there was a build-farm failure on the machine 'komodoensis' [1]
# Failed test 'check replicated rows to tab_rowfilter_toast'
# at t/028_row_filter.pl line 687.
# got: ''
# expected: 't|1'
# Looks like you failed 1 test of 20.
[18:21:24] t/028_row_filter.pl
On Thu, Feb 24, 2022 at 7:43 AM Shinoda, Noriyoshi (PN Japan FSIP)
wrote:
>
> Hi,
> Thank you for developing of the great feature.
> If multiple tables are specified when creating a PUBLICATION,
> is it supposed that the WHERE clause condition is given to only one table?
>
You can give it for
ebruary 23, 2022 11:06 AM
To: Euler Taveira
Cc: houzj.f...@fujitsu.com; Peter Smith ; Alvaro Herrera
; Greg Nancarrow ; vignesh C
; Ajin Cherian ;
tanghy.f...@fujitsu.com; Dilip Kumar ; Rahila Syed
; Peter Eisentraut ;
Önder Kalacı ; japin ; Michael
Paquier ; David Steele ; Craig Ringer
; Amit Lango
On Tue, Feb 22, 2022 at 4:47 AM Euler Taveira wrote:
>
> On Thu, Feb 17, 2022, at 3:36 AM, Amit Kapila wrote:
>
> As there is a new version, I would like to wait for a few more days
> before committing. I am planning to commit this early next week (by
> Tuesday) unless others or I see any more
On Thu, Feb 17, 2022, at 3:36 AM, Amit Kapila wrote:
> As there is a new version, I would like to wait for a few more days
> before committing. I am planning to commit this early next week (by
> Tuesday) unless others or I see any more things that can be improved.
Amit, I don't have additional
On Thu, Feb 17, 2022 at 5:37 PM Amit Kapila wrote:
...
> As there is a new version, I would like to wait for a few more days
> before committing. I am planning to commit this early next week (by
> Tuesday) unless others or I see any more things that can be improved.
I have no more review
On Thu, Feb 10, 2022 at 9:29 AM houzj.f...@fujitsu.com
wrote:
>
>
> Attach the V80 patch which addressed the above comments and
> comments from Amit[1].
>
Thanks for the new version. Few minor/cosmetic comments:
1. Can we slightly change the below comment:
Before:
+ * To avoid fetching the
On Tue, Jan 25, 2022 at 2:18 PM houzj.f...@fujitsu.com
wrote:
>
...
> > 4. src/backend/utils/cache/relcache.c - RelationBuildPublicationDesc
> >
> > - if (relation->rd_pubactions)
> > + if (relation->rd_pubdesc)
> > {
> > - pfree(relation->rd_pubactions);
> > - relation->rd_pubactions = NULL;
On Wed, Feb 9, 2022 at 7:07 AM Peter Smith wrote:
>
> 2. src/backend/commands/publicationcmds.c -
> contain_mutable_or_ud_functions_checker
>
> +/* check_functions_in_node callback */
> +static bool
> +contain_mutable_or_user_functions_checker(Oid func_id, void *context)
> +{
> + return
I did a review of the v79 patch. Below are my review comments:
==
1. doc/src/sgml/ref/create_publication.sgml - CREATE PUBLICATION
The commit message for v79-0001 says:
If your publication contains a partitioned table, the publication parameter
publish_via_partition_root determines if it
On Tue, Feb 8, 2022 at 8:01 AM houzj.f...@fujitsu.com
wrote:
>
> >
> > 12. src/backend/replication/pgoutput/pgoutput.c - get_rel_sync_entry
> >
> > + /*
> > + * Initialize the row filter after getting the final publish_as_relid
> > + * as we only evaluate the row filter of the relation which we
On Mon, Feb 7, 2022 at 1:21 PM Peter Smith wrote:
>
> 5. src/backend/commands/publicationcmds.c - IsRowFilterSimpleExpr (Simple?)
>
> +/*
> + * Is this a simple Node permitted within a row filter expression?
> + */
> +static bool
> +IsRowFilterSimpleExpr(Node *node)
> +{
>
> A lot has changed in
Hi - I did a review of the v77 patches merged with Amit's v77 diff patch [1].
(Maybe this is equivalent to reviewing v78)
Below are my review comments:
==
1. doc/src/sgml/ref/create_publication.sgml - CREATE PUBLICATION
+ The WHERE clause allows simple expressions that
don't have
+
On Fri, Feb 4, 2022 at 2:58 PM houzj.f...@fujitsu.com
wrote:
>
> On Thursday, February 3, 2022 11:11 PM houzj.f...@fujitsu.com
>
>
> Since the v76--clean-up-pgoutput-cache-invalidation.patch has been
> committed, attach a new version patch set to make the cfbot happy. Also
> addressed the
Hi,
On 2022-02-01 13:31:36 +1100, Peter Smith wrote:
> TEST STEPS - Workload case a
>
> 1. Run initdb pub and sub and start both postgres instances (use the nosync
> postgresql.conf)
>
> 2. Run psql for both instances and create tables
> CREATE TABLE test (key int, value text, data jsonb,
On Sat, Jan 29, 2022 at 11:31 AM Andres Freund wrote:
>
> Hi,
>
> Are there any recent performance evaluations of the overhead of row filters? I
> think it'd be good to get some numbers comparing:
>
> 1) $workload with master
> 2) $workload with patch, but no row filters
> 3) $workload with
Hi Peter,
I just tried scenario b that Andres suggested:
For scenario b, I did some testing with row-filter-patch v74 and
various levels of filtering. 0% replicated to 100% rows replicated.
The times are in seconds, I did 5 runs each.
Results:
RUN HEAD "with patch 0%" "row-filter-patch
On Tue, Feb 1, 2022 at 4:51 PM Amit Kapila wrote:
>
> Review Comments:
> ===
> 1.
> + else if (IsA(node, OpExpr))
> + {
> + /* OK, except user-defined operators are not allowed. */
> + if (((OpExpr *) node)->opno >= FirstNormalObjectId)
> + errdetail_msg = _("User-defined operators
On Tue, Feb 1, 2022 at 9:15 AM houzj.f...@fujitsu.com
wrote:
>
> On Monday, January 31, 2022 9:02 PM Amit Kapila
> >
Review Comments:
===
1.
+ else if (IsA(node, OpExpr))
+ {
+ /* OK, except user-defined operators are not allowed. */
+ if (((OpExpr *) node)->opno >=
On Tue, Feb 1, 2022 at 2:45 PM houzj.f...@fujitsu.com <
houzj.f...@fujitsu.com> wrote:
>
> On Monday, January 31, 2022 9:02 PM Amit Kapila
> >
> > 2.
> > + if (pubrinfo->pubrelqual)
> > + appendPQExpBuffer(query, " WHERE (%s)", pubrinfo->pubrelqual);
> > + appendPQExpBufferStr(query, ";\n");
> >
On Tue, Feb 1, 2022 at 9:15 AM houzj.f...@fujitsu.com
wrote:
>
> On Monday, January 31, 2022 9:02 PM Amit Kapila
> >
>
> > 3.
> > + /* row filter (if any) */
> > + if (pset.sversion >= 15)
> > + {
> > + if (!PQgetisnull(result, i, 1))
> > + appendPQExpBuffer(, " WHERE %s", PQgetvalue(result,
On Tue, Feb 1, 2022 at 2:45 PM houzj.f...@fujitsu.com
wrote:
>
> Attach the V75 patch set which address the above, Amit's[1] and Greg's[2][3]
> comments.
>
In the v74-0001 patch (and now in the v75-001 patch) a change was made
in the GetTopMostAncestorInPublication() function, to get the
> On Saturday, January 29, 2022 8:31 AM Andres Freund
> wrote:
> >
> > Hi,
> >
> > Are there any recent performance evaluations of the overhead of row
> > filters? I think it'd be good to get some numbers comparing:
>
> Thanks for looking at the patch! Will test it.
>
> > > case
On Mon, Jan 31, 2022 at 12:57 PM houzj.f...@fujitsu.com
wrote:
>
> Attach the V74 patch set which did the following changes:
>
In the v74-0001 patch, I noticed the following code in get_rel_sync_entry():
+ /*
+ * Tuple slots cleanups. (Will be rebuilt later if needed).
+ */
+ oldctx =
On Sat, Jan 29, 2022 at 6:01 AM Andres Freund wrote:
>
>
> > + if (has_filter)
> > + {
> > + /* Create or reset the memory context for row filters */
> > + if (entry->cache_expr_cxt == NULL)
> > + entry->cache_expr_cxt =
> >
On Tue, Feb 1, 2022 at 12:07 PM Peter Smith wrote:
>
> On Sat, Jan 29, 2022 at 11:31 AM Andres Freund wrote:
> >
> > Hi,
> >
> > Are there any recent performance evaluations of the overhead of row
> > filters? I
> > think it'd be good to get some numbers comparing:
> >
> > 1) $workload with
On 2022-01-31 14:12:38 +1100, Greg Nancarrow wrote:
> This array was only ever meant to be read-only, and visible only to
> that function.
> IMO removing "static" makes things worse because now that array gets
> initialized each call to the function, which is unnecessary.
> I think it should just
On Mon, Jan 31, 2022 at 1:08 PM Amit Kapila wrote:
>
> On Mon, Jan 31, 2022 at 7:27 AM houzj.f...@fujitsu.com
> wrote:
> >
> > On Monday, January 31, 2022 8:53 AM Peter Smith
> > wrote:
> > >
> > > PSA v73*.
> > >
> > > (A rebase was needed due to recent changes in tab-complete.c.
> > >
On Mon, Jan 31, 2022 at 7:27 AM houzj.f...@fujitsu.com
wrote:
>
> On Monday, January 31, 2022 8:53 AM Peter Smith wrote:
> >
> > PSA v73*.
> >
> > (A rebase was needed due to recent changes in tab-complete.c.
> > Otherwise, v73* is the same as v72*).
>
> Thanks for the rebase.
> Attach the V74
On Mon, Jan 31, 2022 at 12:57 PM houzj.f...@fujitsu.com
wrote:
>
> Attach the V74 patch set which did the following changes:
>
Hi,
I tested psql and pg_dump after application of this patch, from the
following perspectives:
- "\dRp+" and "\d " (added by the patch, for PostgreSQL
15) show row
On Mon, Jan 31, 2022 at 1:12 PM houzj.f...@fujitsu.com
wrote:
>
> > > + /*
> > > +* We need this map to avoid relying on ReorderBufferChangeType
> > enums
> > > +* having specific values.
> > > +*/
> > > + static int map_changetype_pubaction[] = {
> > > +
On Saturday, January 29, 2022 8:31 AM Andres Freund wrote:
>
> Hi,
>
> Are there any recent performance evaluations of the overhead of row filters? I
> think it'd be good to get some numbers comparing:
Thanks for looking at the patch! Will test it.
> 1) $workload with master
> 2) $workload
On 2022-Jan-28, Andres Freund wrote:
> > + foreach(lc, data->publications)
> > + {
> > + Publication *pub = lfirst(lc);
...
> Isn't this basically O(schemas * publications)?
Yeah, there are various places in the logical replication code that seem
pretty careless about this kind
Hi,
Are there any recent performance evaluations of the overhead of row filters? I
think it'd be good to get some numbers comparing:
1) $workload with master
2) $workload with patch, but no row filters
3) $workload with patch, row filter matching everything
4) $workload with patch, row filter
I just pushed a change to tab-complete because of a comment in the
column-list patch series. I checked and your v72-0002 does not
conflict, but it doesn't fully work either; AFAICT you'll have to change
it so that the WHERE clause appears in the COMPLETE_WITH(",") line I
just added. As far as I
On Fri, Jan 28, 2022 at 2:26 PM houzj.f...@fujitsu.com
wrote:
>
> Attach the V72 patch set which did the above changes.
>
Thanks for updating the patch set.
One thing I noticed, in the patch commit comment it says:
Psql commands \dRp+ and \d will display any row filters.
However, "\d" by
Here are some review comments for v71-0001
~~~
1. Commit Message - database
"...that don't satisfy this WHERE clause will be filtered out. This allows a
database or set of tables to be partially replicated. The row filter is
per table. A new row filter can be added simply by specifying a
On Thu, Jan 27, 2022 at 4:59 PM Peter Smith wrote:
>
> On Thu, Jan 27, 2022 at 9:40 AM Greg Nancarrow
wrote:
> >
> > On Wed, Jan 26, 2022 at 2:08 PM houzj.f...@fujitsu.com
> > wrote:
> > >
> > > There was a miss in the posted patch which didn't initialize the
parameter in
> > >
On Thu, Jan 27, 2022 at 9:40 AM Greg Nancarrow wrote:
>
> On Wed, Jan 26, 2022 at 2:08 PM houzj.f...@fujitsu.com
> wrote:
> >
> > There was a miss in the posted patch which didn't initialize the parameter
> > in
> > RelationBuildPublicationDesc, sorry for that. Attach the correct patch this
>
On Wed, Jan 26, 2022 at 2:08 PM houzj.f...@fujitsu.com
wrote:
>
> There was a miss in the posted patch which didn't initialize the parameter in
> RelationBuildPublicationDesc, sorry for that. Attach the correct patch this
> time.
>
I have some additional doc update suggestions for the v71-0001
On Wed, Jan 26, 2022 at 2:08 PM houzj.f...@fujitsu.com
wrote:
>
> There was a miss in the posted patch which didn't initialize the parameter in
> RelationBuildPublicationDesc, sorry for that. Attach the correct patch this
> time.
>
A few comments for the v71-0001 patch:
On Tue, Jan 25, 2022 at 2:18 PM houzj.f...@fujitsu.com
wrote:
>
> On Monday, January 24, 2022 4:38 PM Peter Smith
> >
...
> > 5. src/include/catalog/pg_publication.h - typedef struct PublicationDesc
> >
> > +typedef struct PublicationDesc
> > +{
> > + /*
> > + * true if the columns referenced in
On Wed, Jan 26, 2022 at 8:37 AM houzj.f...@fujitsu.com
wrote:
>
> On Monday, January 24, 2022 4:38 PM Peter Smith wrote:
> >
> >
> > 3. src/backend/utils/cache/relcache.c - RelationBuildPublicationDesc
> >
> > +RelationBuildPublicationDesc(Relation relation)
> > {
> > List*puboids;
> >
On Monday, January 24, 2022 4:38 PM Peter Smith wrote:
>
> Thanks for all the patches!
>
> Here are my review comments for v69-0001
>
> ~~~
>
> 1. src/backend/executor/execReplication.c CheckCmdReplicaIdentity call to
> RelationBuildPublicationDesc
>
> + /*
> + * It is only safe to execute
On Monday, January 24, 2022 5:36 AM Peter Smith
>
> FYI - I noticed the cfbot is reporting a failed test case [1] for the latest
> v69 patch
> set.
>
> [21:09:32.183] # Failed test 'check replicated inserts on subscriber'
> [21:09:32.183] # at t/025_rep_changes_for_schema.pl line 202.
>
A couple more comments for the v69-0001 TAP tests.
~~~
1. src/test/subscription/t/027_row_filter.pl
+# The subscription of the ALL TABLES IN SCHEMA publication means
there should be
+# no filtering on the tablesync COPY, so all expect all 5 will be present.
+$result =
On Mon, Jan 24, 2022 at 1:19 PM Greg Nancarrow wrote:
>
> On Mon, Jan 24, 2022 at 5:09 PM Amit Kapila wrote:
> > But that is not what I am seeing in Logs with a test case where the
> > row filter column has NULL values. Could you please try that see what
> > is printed in LOG?
> >
> > You can
Thanks for all the patches!
Here are my review comments for v69-0001
~~~
1. src/backend/executor/execReplication.c CheckCmdReplicaIdentity
call to RelationBuildPublicationDesc
+ /*
+ * It is only safe to execute UPDATE/DELETE when all columns, referenced in
+ * the row filters from
On Mon, Jan 24, 2022 at 5:09 PM Amit Kapila wrote:
>
> On Mon, Jan 24, 2022 at 10:29 AM Greg Nancarrow wrote:
> >
> > On Mon, Jan 24, 2022 at 2:47 PM Amit Kapila wrote:
> > >
> > > > (3) pgoutput_row_filter_exec_expr
> > > > pgoutput_row_filter_exec_expr() returns false if "isnull" is true,
> >
On Mon, Jan 24, 2022 at 10:29 AM Greg Nancarrow wrote:
>
> On Mon, Jan 24, 2022 at 2:47 PM Amit Kapila wrote:
> >
> > > (3) pgoutput_row_filter_exec_expr
> > > pgoutput_row_filter_exec_expr() returns false if "isnull" is true,
> > > otherwise (if "isnull" is false) returns the value of "ret"
> >
On Mon, Jan 24, 2022 at 4:53 PM Peter Smith wrote:
>
> On Fri, Jan 21, 2022 at 2:04 PM Amit Kapila wrote:
> >
> > On Thu, Jan 20, 2022 at 7:56 PM Alvaro Herrera
> > wrote:
> > >
> > > > > Maybe this was meant to be "validate RF
> > > > > expressions" and return, perhaps, a bitmapset of all
On Fri, Jan 21, 2022 at 2:04 PM Amit Kapila wrote:
>
> On Thu, Jan 20, 2022 at 7:56 PM Alvaro Herrera
> wrote:
> >
> > > > Maybe this was meant to be "validate RF
> > > > expressions" and return, perhaps, a bitmapset of all invalid columns
> > > > referenced?
> > >
> > > Currently, we stop as
On Mon, Jan 24, 2022 at 8:36 AM Peter Smith wrote:
>
> FYI - I noticed the cfbot is reporting a failed test case [1] for the
> latest v69 patch set.
>
> [21:09:32.183] # Failed test 'check replicated inserts on subscriber'
> [21:09:32.183] # at t/025_rep_changes_for_schema.pl line 202.
>
On Mon, Jan 24, 2022 at 2:47 PM Amit Kapila wrote:
>
> > (3) pgoutput_row_filter_exec_expr
> > pgoutput_row_filter_exec_expr() returns false if "isnull" is true,
> > otherwise (if "isnull" is false) returns the value of "ret"
> > (true/false).
> > So the following elog needs to be changed (Peter
On Fri, Jan 21, 2022 at 2:56 PM Greg Nancarrow wrote:
>
> On Thu, Jan 20, 2022 at 12:12 PM houzj.f...@fujitsu.com
> wrote:
>
> (3) pgoutput_row_filter_exec_expr
> pgoutput_row_filter_exec_expr() returns false if "isnull" is true,
> otherwise (if "isnull" is false) returns the value of "ret"
>
On Sat, Jan 22, 2022 at 8:45 PM Alvaro Herrera wrote:
>
> On 2022-Jan-22, Amit Kapila wrote:
>
> > CREATE TABLE parent (a int primary key, b int not null, c varchar)
> > PARTITION BY RANGE(a);
> > CREATE TABLE child PARTITION OF parent FOR VALUES FROM (0) TO (250);
> > CREATE UNIQUE INDEX b_index
FYI - I noticed the cfbot is reporting a failed test case [1] for the
latest v69 patch set.
[21:09:32.183] # Failed test 'check replicated inserts on subscriber'
[21:09:32.183] # at t/025_rep_changes_for_schema.pl line 202.
[21:09:32.183] # got: '21|1|2139062143'
[21:09:32.183] # expected:
On 2022-Jan-22, Amit Kapila wrote:
> CREATE TABLE parent (a int primary key, b int not null, c varchar)
> PARTITION BY RANGE(a);
> CREATE TABLE child PARTITION OF parent FOR VALUES FROM (0) TO (250);
> CREATE UNIQUE INDEX b_index on child(b);
> ALTER TABLE child REPLICA IDENTITY using INDEX
On Fri, Jan 21, 2022 at 8:19 PM Alvaro Herrera wrote:
>
> If ATTACH PARTITION or CREATE TABLE .. PARTITION OF don't let you
> specify replica identity, I suspect it's because both partitioning and
> logical replication were developed in parallel, and neither gave too
> much thought to the other.
On 2022-Jan-21, houzj.f...@fujitsu.com wrote:
> Personally, I'm a little hesitant to put the check at DDL level, because
> adding check at DDLs like ATTACH PARTITION/CREATE PARTITION OF ( [1]
> explained why we need to check these DDLs) looks a bit restrictive and
> user might also complain about
On Thu, Jan 20, 2022 at 4:54 PM Amit Kapila wrote:
> + /*
> + * Unchanged toasted replica identity columns are only detoasted in the
> + * old tuple, copy this over to the new tuple.
> + */
> + if (att->attlen == -1 &&
> + VARATT_IS_EXTERNAL_ONDISK(tmp_new_slot->tts_values[i]) &&
> +
On Thur, Jan 20, 2022 7:25 PM Amit Kapila wrote:
>
> On Thu, Jan 20, 2022 at 6:42 AM houzj.f...@fujitsu.com
> wrote:
> >
> > Attach the V68 patch set which addressed the above comments and changes.
> >
>
> Few comments and suggestions:
> ==
> 1.
> /*
> + * For updates,
On Thu, Jan 20, 2022 at 12:12 PM houzj.f...@fujitsu.com
wrote:
>
> Attach the V68 patch set which addressed the above comments and changes.
> The version patch also fix the error message mentioned by Greg[1]
>
Some review comments for the v68 patch, mostly nitpicking:
(1) Commit message
Minor
On Thur, Jan 20, 2022 10:26 PM Alvaro Herrera wrote:
>
> On 2022-Jan-20, Amit Kapila wrote:
>
> > It returns an invalid column referenced in an RF if any but if not
> > then it helps to form pubactions which is anyway required at a later
> > point in the caller. The idea is that when we are
On Thursday, January 20, 2022 9:14 PM Alvaro Herrera
wrote:
>
> I was skimming this and the changes in CheckCmdReplicaIdentity caught my
> attention. "Is this code running at the publisher side or the subscriber
> side?" I
> wondered -- because the new error messages being added look intended
On Thu, Jan 20, 2022 at 7:56 PM Alvaro Herrera wrote:
>
> > > Maybe this was meant to be "validate RF
> > > expressions" and return, perhaps, a bitmapset of all invalid columns
> > > referenced?
> >
> > Currently, we stop as soon as we find the first invalid column.
>
> That seems quite strange.
On Fri, Jan 21, 2022 at 12:13 AM Alvaro Herrera
wrote:
>
> And while wondering about that, I stumbled upon
> GetRelationPublicationActions(), which has a very weird API that it
> always returns a palloc'ed block -- but without saying so. And
> therefore, its only caller leaks that memory. Maybe
On 2022-Jan-20, Amit Kapila wrote:
> It returns an invalid column referenced in an RF if any but if not
> then it helps to form pubactions which is anyway required at a later
> point in the caller. The idea is that when we are already traversing
> publications we should store/gather as much info
On Thu, Jan 20, 2022 at 6:43 PM Alvaro Herrera wrote:
>
> And the actual reason I was looking at this code, is that I had stumbled
> upon the new GetRelationPublicationInfo() function, which has an even
> weirder API:
>
> > * Get the publication information for the given relation.
> > *
> > *
I was skimming this and the changes in CheckCmdReplicaIdentity caught my
attention. "Is this code running at the publisher side or the subscriber
side?" I wondered -- because the new error messages being added look
intended to be thrown at the publisher side; but the existing error
messages
On Thu, Jan 20, 2022 at 5:03 PM tanghy.f...@fujitsu.com
wrote:
>
> On Thu, Jan 20, 2022 9:13 AM houzj.f...@fujitsu.com
> wrote:
> > Attach the V68 patch set which addressed the above comments and changes.
> > The version patch also fix the error message mentioned by Greg[1]
> >
>
> I saw a
On Thu, Jan 20, 2022 9:13 AM houzj.f...@fujitsu.com
wrote:
> Attach the V68 patch set which addressed the above comments and changes.
> The version patch also fix the error message mentioned by Greg[1]
>
I saw a problem about this patch, which is related to Replica Identity check.
For
On Thu, Jan 20, 2022 at 6:42 AM houzj.f...@fujitsu.com
wrote:
>
> Attach the V68 patch set which addressed the above comments and changes.
>
Few comments and suggestions:
==
1.
/*
+ * For updates, if both the new tuple and old tuple are not null, then both
+ * of them
On Thu, Jan 20, 2022 at 2:29 PM Amit Kapila wrote:
>
> On Thu, Jan 20, 2022 at 7:51 AM Peter Smith wrote:
> >
> > Here are some review comments for v68-0001.
> >
> >
> > 3. src/backend/replication/pgoutput/pgoutput.c - pgoutput_row_filter_init
> >
> > + /* If no filter found, clean up the memory
1 - 100 of 623 matches
Mail list logo
