Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

the expression, we will use sequential scan >> >> > > Thanks, fixed > > I'll add the changes to v49 in the next e-mail. > It seems you forgot to address these last two comments in the latest version. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

test file was inconsistently using ';' while executing statement with safe_psql, changed it to remove ';'. -- With Regards, Amit Kapila. v48-0001-Allow-the-use-of-indexes-other-than-PK-and-REPLI.patch Description: Binary data

Re: Allow logical replication to copy tables in binary format

ould better to write the tests for this feature in the existing test file 014_binary as that would save some time for node setup/shutdown and also that would be a more appropriate place for these tests. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

that it will pick up a sequence scan. However, just do the poll for the number of index scans stat once. I think that will cover the case you are worried about without having a noticeable impact on test timing. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

On Mon, Mar 13, 2023 at 2:44 AM Peter Smith wrote: > > On Sat, Mar 11, 2023 at 6:05 PM Amit Kapila wrote: > > > > On Fri, Mar 10, 2023 at 7:58 PM Önder Kalacı wrote: > > > > > > > > > I think one option could be to drop some ca

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

ci Reviewed-by: Peter Smith, Shi yu, Hou Zhijie, Vignesh C, Kuroda Hayato, Amit Kapila Discussion: https://postgr.es/m/CACawEhVLqmAAyPXdHEPv1ssU2c=dqoniigz7g73hfys7+ng...@mail.gmail.com -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

ata is replicated but I feel it may still be better to use wait_for_catchup(). Similarly, wait_for_subscription_sync uses the publisher name and appname in other tests, so it is better to be consistent. It can avoid random failures by ensuring data is synced. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

ould be better to report and discuss this in a separate thread, > Attached as v40_0001 on the patch. > > Note that I need to have that commit as 0001 so that 0002 patch > passes the tests. > I think we can add such a test (which relies on existing buggy behavior) later after fixing the existing bug. For now, it would be better to remove that test and add it after we fix dropped columns issue in HEAD. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

index > is not sent over the wire from the pub, we can prefer the sequential scan. > > Is my understanding of your suggestion accurate? > Yes. I request an opinion from Shi-San who has reported the problem. -- With Regards, Amit Kapila.

Re: Add macros for ReorderBufferTXN toptxn

does this. > I also find it will make code easier to read. So, +1 to the idea. I'll do the detailed review and test next week unless there are objections to the idea. -- With Regards, Amit Kapila.

Re: Rework LogicalOutputPluginWriterUpdateProgress

On Fri, Mar 10, 2023 at 11:17 AM Peter Smith wrote: > > On Fri, Mar 10, 2023 at 3:32 PM Amit Kapila wrote: > > > > On Thu, Mar 9, 2023 at 10:56 AM Peter Smith wrote: > > > > > > 2. rollback_prepared_cb_wrapper > > > > > > /* > >

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

On Thu, Mar 9, 2023 at 5:47 PM Önder Kalacı wrote: > > Amit Kapila , 8 Mar 2023 Çar, 14:42 tarihinde şunu > yazdı: >> >> On Wed, Mar 8, 2023 at 4:51 PM Önder Kalacı wrote: >> > >> > >> >> >> >> I just share this case and then w

Re: Rework LogicalOutputPluginWriterUpdateProgress

@@ stream_abort_cb_wrapper(ReorderBuffer *cache, > ReorderBufferTXN *txn, > > /* Pop the error context stack */ > error_context_stack = errcallback.previous; > + > + UpdateProgressAndKeepalive(ctx, (txn->toptxn == NULL)); > } > > ~ > > Are the double parentheses necessary? > Personally, I find this style easier to follow. -- With Regards, Amit Kapila.

Re: Rework LogicalOutputPluginWriterUpdateProgress

do you have an opinion on the new name used in the patch? If we agree that we don't need to backpatch for (1) and the new name for (4) is reasonable then we can commit 1-4 as one patch and then look at the remaining patch. Thoughts? -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

On Thu, Mar 9, 2023 at 4:50 PM Amit Kapila wrote: > > On Thu, Mar 9, 2023 at 3:26 PM Önder Kalacı wrote: > > > > > > So, I changed the first one to SUBSCRIPTION USES INDEX WITH MULTIPLE ROWS > > AND COLUMNS > > and dropped the second one. Let me know if it d

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

xrelname = 'test_replica_id_full_idy';} +) or die "Timed out while waiting for creating index test_replica_id_full_idy"; It appears you are using inconsistent spacing. It may be better to use a single empty line wherever required. [1] - https://www.postgresql.org/message-id/CAA4eK1%2BoM_v-b_WDHZmqCyVHU2oD4j3vF9YcH9xVHj%3DzAfy4og%40mail.gmail.com -- With Regards, Amit Kapila.

Re: Time delayed LR (WAS Re: logical replication restrictions)

On Thu, Mar 9, 2023 at 2:56 PM Kyotaro Horiguchi wrote: > > At Thu, 9 Mar 2023 11:00:46 +0530, Amit Kapila > wrote in > > On Wed, Mar 8, 2023 at 9:20 AM Masahiko Sawada > > wrote: > > > > > > On Wed, Mar 8, 2023 at 3:30 AM Nathan Bossart > > &g

Re: pg_upgrade and logical replication

On Wed, Mar 8, 2023 at 12:26 PM Julien Rouhaud wrote: > > On Sat, 4 Mar 2023, 14:13 Amit Kapila, wrote: >> >> >> > For the publisher nodes, that may be something nice to support (I'm >> > assuming it >> > could be useful for more complex rep

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

On Thu, Mar 9, 2023 at 10:37 AM Peter Smith wrote: > > On Thu, Mar 9, 2023 at 3:49 PM Amit Kapila wrote: > > > > On Wed, Mar 8, 2023 at 8:44 PM Önder Kalacı wrote: > > > > > >> > > >> I felt that once you remove the create publication/subscrip

Re: Time delayed LR (WAS Re: logical replication restrictions)

walreceiver process as we have for standby. I think we need to analyze the pros and cons of each of those approaches and see if they would be useful even for other things on the apply side. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

we have removed enable_indexscan check, you should remove this test. 5. In general, the line length seems to vary a lot for different multi-line comments. Though we are not strict in that it will look better if there is consistency in that (let's have ~80 cols line length for each comment in a single line). -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

ith > 'opfamily' declaration. Anyway, I've given my reason a couple of times > now, so if you don't want to change it I won't about it debate > anymore. > I agree with this reasoning. -- With Regards, Amit Kapila.

Re: Allow logical replication to copy tables in binary format

On Wed, Mar 8, 2023 at 6:17 PM Melih Mutlu wrote: > > On 7 Mar 2023 Tue at 04:10 Amit Kapila wrote: >> >> As per what I could read in this thread, most people prefer to use the >> existing binary option rather than inventing a new way (option) to >> binary copy in

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

tch complex. However, it may be better to give it a try and see if this or other regression/optimization can be avoided without adding much complexity to the patch. You can prepare a top-up patch and then we can discuss it. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

le is not used, you might as well remove the > if/else and simplify the code. > Hmm, but that is an existing style/code, and this patch has done nothing which requires that change. Personally, I find the current style better for the readability purpose. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

PTION postgres=# select * from tab1; a --- 2 (1 row) I have debugged the above example and it uses an index scan during apply without your latest change which is what I expected. AFAICS, the use of IdxIsRelationIdentityOrPK() is to decide whether we will do tuples_equal() or not during the index scan and I see it gives the correct results with the example you provided. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

On Tue, Mar 7, 2023 at 3:00 PM Peter Smith wrote: > > On Tue, Mar 7, 2023 at 8:01 PM Amit Kapila wrote: > > > > On Tue, Mar 7, 2023 at 1:19 PM Peter Smith wrote: > > > > > > Let me give an example to demonstrate why I thought something is fishy > >

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

an be. (??) > The difference is that you are misunderstanding the intent of this function. GetRelationIdentityOrPK() returns a "replica identity index oid" if the same is defined, else return PK, if that is defined, otherwise, return invalidOid. This is what is expected by its callers. Now, one can argue to have a different function name and that may be a valid argument but as far as I can see the function does what is expected from it. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

Apart from the above, I have made some modifications in the other comments. If you are convinced with those, then kindly include them in the next version. -- With Regards, Amit Kapila. use_index_sub_amit_1.patch Description: Binary data

Re: Allow logical replication to copy tables in binary format

this is the last CF for this release and we have a limited time left. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

On Tue, Mar 7, 2023 at 1:34 AM Andres Freund wrote: > > On 2023-03-01 14:10:07 +0530, Amit Kapila wrote: > > On Wed, Mar 1, 2023 at 12:09 AM Andres Freund wrote: > > > > > > > I see this as a way to provide this feature for users but I would > > > > p

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

On Thu, Mar 2, 2023 at 2:45 PM Amit Kapila wrote: > > On Thu, Mar 2, 2023 at 1:37 PM shiy.f...@fujitsu.com > wrote: > > - results of `gprof` > > case1: > > master > > % cumulative self self total > > time seconds secondsc

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

tity full. This will still be win in many cases and we are planning to provide a knob to disable this feature. [1] - https://www.postgresql.org/message-id/3466340.1673117404%40sss.pgh.pa.us -- With Regards, Amit Kapila.

Re: Deduplicate logicalrep_read_tuple()

On Sun, Mar 5, 2023 at 1:22 PM Peter Smith wrote: > > On Fri, Mar 3, 2023 at 10:04 PM Amit Kapila wrote: > > > > > > > > Thanks. Done so in the attached v2. > > > > > > > LGTM. Unless Peter or someone has any comments on this, I'll push this &

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

looking at this comment and the fix for it. It seems to me that it would be better to not add the check (indexlist != NIL) here and rather get the indexlist in FindUsableIndexForReplicaIdentityFull(). It will anyway return InvalidOid, if there is no index and that way code will look a bit cleaner. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

On Mon, Mar 6, 2023 at 1:40 PM Peter Smith wrote: > > On Mon, Mar 6, 2023 at 5:44 PM Amit Kapila wrote: > > > > On Mon, Mar 6, 2023 at 10:12 AM Peter Smith wrote: > > > > > > 4. IdxIsRelationIdentityOrPK > > > > > > +/* > &

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

comprising the same or fewer columns must also be set on the subscriber - side. See for details on + the key. When replica identity FULL is specified, + indexes can be used on the subscriber side for searching the rows. These Shouldn't specifying FULL be consistent wih existing docs? -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

LogicalRepPartMapContext which would be a long-lived context and we allocate memory for indexes in FindLogicalRepUsableIndex() which can accumulate over a period of time. So, I think it would be better to switch to the old context in logicalrep_partition_open() before invoking FindLogicalRepUsableIndex() provided that is not a long-lived context. -- With Regards, Amit Kapila.

Re: pg_upgrade and logical replication

On Thu, Mar 2, 2023 at 4:21 PM Julien Rouhaud wrote: > > On Thu, Mar 02, 2023 at 03:47:53PM +0530, Amit Kapila wrote: > > > > Why don't we try to support the direct upgrade of logical replication > > nodes? Have you tried to analyze what are the obstacles and whether w

Re: Fix comments in gistxlogDelete, xl_heap_freeze_page and xl_btree_delete

ing forward to your feedback, > > > > Regards, > > > > It looks like I did not create a CF entry for this one: fixed with [1]. > > Also, while at it, adding a commit message in V2 attached. > LGTM. -- With Regards, Amit Kapila.

Re: Deduplicate logicalrep_read_tuple()

ould that comment be modified to say > > /* not strictly necessary for LOGICALREP_COLUMN_BINARY but per > > StringInfo practice */ > > Thanks. Done so in the attached v2. > LGTM. Unless Peter or someone has any comments on this, I'll push this early next week. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

at this was an unnecessary change. Anyway, I see your point, so if you want to keep the naming as you proposed at least don't change the ordering for get_opclass_input_type() call because that looks odd to me. > > Attached v29 for this review. Note that I'll be working on the disable index > scan changes after > Okay, thanks! -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

or the current usage of the patch. 8. + TypeCacheEntry **eq = NULL; /* only used when the index is not RI or PK */ Normally, we don't add such comments as the usage is quite obvious by looking at the code. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

ad rows) some index has without more planner support. Personally, I feel it is better to have a table-level option for this so that users have some knob to avoid regressions in particular cases. In general, I agree that it will be a win in more number of cases than it can regress. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

hat they can be supported in the future if it is feasible to do so. > > Attached are both patches: the main patch, and the patch that > optionally disables the index scans. > Both the patches are numbered 0001. It would be better to number them as 0001 and 0002. -- With Regards, Amit Kapila.

Re: pg_upgrade and logical replication

On Wed, Mar 1, 2023 at 12:25 PM Julien Rouhaud wrote: > > On Wed, Mar 01, 2023 at 11:51:49AM +0530, Amit Kapila wrote: > > On Tue, Feb 28, 2023 at 10:18 AM Julien Rouhaud wrote: > > > > > > > Okay, but it would be better if you list out your detailed steps.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

with the index scan. Because the update will generate dead tuples in the same transaction and those dead tuples won't be removed, we get those from the index and then need to perform index_fetch_heap() to find out whether the tuple is dead or not. Now, for sequence scan also we need to scan those dead tuples but there we don't need to do back-and-forth between index and heap. I think we can once check with more number of tuples (say with 2, 5, etc.) for case-1. -- With Regards, Amit Kapila.

Re: Should vacuum process config file reload more often

t; > the beginning of lazy vacuum, but perhaps we would need to > > enlarge/shrink it based on the new value? > > I don't think we need to do anything about that initially, just because the > config can be changed in a more granular way, doesn't mean we have to react to > every change for the current operation. > +1. I also don't see the need to do anything for this case. -- With Regards, Amit Kapila.

Re: Allow logical replication to copy tables in binary format

would be more confusing for the user > - unified option = this would be simpler and faster, but risks > breaking existing applications currently using 'binary=true' > I would prefer a unified option as apart from other things you and others mentioned that will be less of a maintenance burden in the future. -- With Regards, Amit Kapila.

Re: Time delayed LR (WAS Re: logical replication restrictions)

be better to tell about this in the docs along with the 'min_send_delay' description. The key point is whether this would be an acceptable trade-off for users who want to use this feature. I think it can harm only if users use this without understanding the corresponding trade-off. As we kept the default to no delay, it is expected from users using this have an understanding of the trade-off. -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

by the logical replication target relation" when it should have given an error: "logical replication target relation \"%s.%s\" has neither REPLICA IDENTITY index nor PRIMARY ... -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

to have an option for this which I think we have an agreement on, now I will continue my review. -- With Regards, Amit Kapila.

Re: pg_upgrade and logical replication

On Tue, Feb 28, 2023 at 10:18 AM Julien Rouhaud wrote: > > On Tue, Feb 28, 2023 at 08:56:37AM +0530, Amit Kapila wrote: > > On Tue, Feb 28, 2023 at 7:55 AM Julien Rouhaud wrote: > > > > > > > > > The scenario I'm interested in is to rely on logical replic

Re: Time delayed LR (WAS Re: logical replication restrictions)

On Wed, Mar 1, 2023 at 10:57 AM Masahiko Sawada wrote: > > On Wed, Mar 1, 2023 at 1:55 PM Amit Kapila wrote: > > > > > Won't a malicious user can block the > > replication in other ways as well and let the publisher stall (or > > crash the publisher) ev

Re: Time delayed LR (WAS Re: logical replication restrictions)

ng the existing comments to something like: "If we've requested to shut down, exit the process. This is unlike handling at other places where we allow complete WAL to be sent before shutdown because we don't want the delayed transactions to be applied downstream. This will allow one to use the data from downstream in case of some unwanted operations on the current node." -- With Regards, Amit Kapila.

Re: Time delayed LR (WAS Re: logical replication restrictions)

se already where during Drop Subscription, if the network is not reachable then also, a similar problem can happen and users need to be careful about it [1]. [1] - https://www.postgresql.org/docs/devel/logical-replication-subscription.html -- With Regards, Amit Kapila.

Re: Time delayed LR (WAS Re: logical replication restrictions)

On Wed, Mar 1, 2023 at 8:06 AM Kyotaro Horiguchi wrote: > > At Tue, 28 Feb 2023 08:35:11 +0530, Amit Kapila > wrote in > > On Tue, Feb 28, 2023 at 8:14 AM Kyotaro Horiguchi > > wrote: > > > > > > At Mon, 27 Feb 2023 14:56:19 +0530, Amit Kapila > >

Re: Time delayed LR (WAS Re: logical replication restrictions)

ere? Apart from the above, I have made a few changes in the comments in the attached diff patch. If you agree with those then please include them in the next version. -- With Regards, Amit Kapila. changes_amit_1.patch Description: Binary data

Re: pg_upgrade and logical replication

On Tue, Feb 28, 2023 at 7:55 AM Julien Rouhaud wrote: > > On Mon, Feb 27, 2023 at 03:39:18PM +0530, Amit Kapila wrote: > > > > BTW, thinking some more > > on this, how will we allow to continue replication after upgrading the > > publisher? During upgr

Re: Time delayed LR (WAS Re: logical replication restrictions)

On Tue, Feb 28, 2023 at 8:14 AM Kyotaro Horiguchi wrote: > > At Mon, 27 Feb 2023 14:56:19 +0530, Amit Kapila > wrote in > > The one difference w.r.t recovery_min_apply_delay is that here we will > > hold locks for the duration of the delay which didn't seem to be a > >

Re: pg_upgrade and logical replication

On Sun, Feb 26, 2023 at 8:35 AM Julien Rouhaud wrote: > > On Sat, Feb 25, 2023 at 11:24:17AM +0530, Amit Kapila wrote: > > > > > > The new command is of the form > > > > > > ALTER SUBSCRIPTION name ADD TABLE (relid = X, state = 'Y', lsn = 'Z/Z') &g

Re: Time delayed LR (WAS Re: logical replication restrictions)

On Mon, Feb 27, 2023 at 1:50 PM Masahiko Sawada wrote: > > On Mon, Feb 27, 2023 at 3:34 PM Amit Kapila wrote: > > > > > --- > > > Why do we not to delay sending COMMIT PREPARED messages? > > > > > > > I think we need to either add de

Re: Allow logical replication to copy tables in binary format

On Mon, Feb 20, 2023 at 3:37 PM shiy.f...@fujitsu.com wrote: > > On Thu, Feb 16, 2023 8:48 PM Amit Kapila wrote: > > > > So, doesn't this mean that there is no separate failure mode during > > the initial copy? I am clarifying this to see if the patch really > >

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

e Parameters" [1] for a table. See parameters like autovacuum_enabled that decide the autovacuum behavior for a table. These can be set via CREATE/ALTER TABLE commands. [1] - https://www.postgresql.org/docs/devel/sql-createtable.html#SQL-CREATETABLE-STORAGE-PARAMETERS -- With Regards, Amit Kapila.

Re: Time delayed LR (WAS Re: logical replication restrictions)

sn't seem to be required. > --- > Why do we not to delay sending COMMIT PREPARED messages? > I think we need to either add delay for prepare or commit prepared as otherwise, it will lead to delaying the xact more than required. The patch seems to add a delay before sending a PREPARE as that is the time when the subscriber will apply the changes. -- With Regards, Amit Kapila.

Re: Time delayed LR (WAS Re: logical replication restrictions)

ine those functionalities into one API. Thoughts? [1] - https://www.postgresql.org/message-id/20230210210423.r26ndnfmuifie4f6%40awork3.anarazel.de -- With Regards, Amit Kapila.

Re: Change xl_hash_vacuum_one_page.ntuples from int to uint16

On Fri, Feb 17, 2023 at 7:44 PM Drouvot, Bertrand wrote: > > On 2/16/23 1:26 PM, Drouvot, Bertrand wrote: > > Hi, > > > > On 2/16/23 12:00 PM, Amit Kapila wrote: > > > >> BTW, feel free to create the second patch > >> (to align the types

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

with this if we can get some more buy-in from senior community members (at least one more committer) and some user(s) if possible. So, I once again request others to chime in and share their opinion. -- With Regards, Amit Kapila.

Re: pg_upgrade and logical replication

On Wed, Feb 22, 2023 at 12:13 PM Julien Rouhaud wrote: > > On Mon, Feb 20, 2023 at 03:07:37PM +0800, Julien Rouhaud wrote: > > On Mon, Feb 20, 2023 at 11:07:42AM +0530, Amit Kapila wrote: > > > > > > I think the current mechanism tries to provide more flexibility to

Re: Time delayed LR (WAS Re: logical replication restrictions)

ed". + There appears to be a special value <0x00> after "ready". I think that is added by mistake or probably you have used some editor which has added this value. Can we slightly reword this to: "However, there is a possibility that the table status updated in pg_subscription_rel could be delayed in getting to the "ready" state, and also two-phase (if specified) could be delayed in getting to "enabled"."? -- With Regards, Amit Kapila.

Re: Time delayed LR (WAS Re: logical replication restrictions)

LogicalDecodingContext > LogicalOutputPluginWriterPrepareWrite prepare_write; > LogicalOutputPluginWriterWrite write; > LogicalOutputPluginWriterUpdateProgress update_progress; > + LogicalOutputPluginWriterDelay delay; > > ~ > > 15a. > Question: Is there some advantage to introducing another callback, > instead of just doing the delay inline? > This is required because we need to check walsender's timeout and or process replies during the delay. -- With Regards, Amit Kapila.

Re: pg_upgrade and logical replication

On Sun, Feb 19, 2023 at 5:31 AM Julien Rouhaud wrote: > > On Sat, Feb 18, 2023 at 04:12:52PM +0530, Amit Kapila wrote: > > > > > > > > > > > > > I also don't know if there is any other safe way for newly added > > > > tables apart

Re: Support logical replication of DDLs

On Sun, Feb 19, 2023 at 7:50 AM Jonathan S. Katz wrote: > > On 2/17/23 4:15 AM, Amit Kapila wrote: > > > This happens because of the function used in the index expression. > > Now, this is not the only thing, the replication can even fail during > > DDL replication w

Re: pg_upgrade and logical replication

On Sat, Feb 18, 2023 at 11:21 AM Julien Rouhaud wrote: > > On Sat, Feb 18, 2023 at 09:31:30AM +0530, Amit Kapila wrote: > > On Fri, Feb 17, 2023 at 9:05 PM Julien Rouhaud wrote: > > > > > > I'm concerned about people not coming from physical replication. If

Re: Time delayed LR (WAS Re: logical replication restrictions)

apply for min_send_delay milliseconds. I think here also comments should be updated as per the changed approach for applying the delay on the publisher side. -- With Regards, Amit Kapila.

Re: pg_upgrade and logical replication

On Fri, Feb 17, 2023 at 9:05 PM Julien Rouhaud wrote: > > On Fri, Feb 17, 2023 at 04:12:54PM +0530, Amit Kapila wrote: > > On Fri, Feb 17, 2023 at 1:24 PM Julien Rouhaud wrote: > > > > > > An easy workaround that I tried is to allow something like > > &g

Re: Change xl_hash_vacuum_one_page.ntuples from int to uint16

On Fri, Feb 17, 2023 at 9:43 AM Andres Freund wrote: > > On 2023-02-17 08:30:09 +0530, Amit Kapila wrote: > > Thanks, I was not completely sure about whether we need to bump > > XLOG_PAGE_MAGIC for this patch as this makes the additional space just > > by chan

Re: pg_upgrade and logical replication

during pg_upgrade) that also restores the pg_subscription_rel > content in each subscription or something like that. If not, should > pg_upgrade > keep preserving the subscriptions as it doesn't seem safe to use them, or at > least document the hazards (I didn't find anything about it in the > documentation)? > > There is a mention of this in pg_dump docs. See [1] (When dumping logical replication subscriptions ...) [1] - https://www.postgresql.org/docs/devel/app-pgdump.html -- With Regards, Amit Kapila.

Re: Support logical replication of global object commands

ould handle the WAL for global objects if we > don't filter by database? Is there any specific problem you see for > decoding global objects commands in a database specific WALSender? > I haven't verified but I was concerned about the below check: logicalddl_decode { ... + + if (message->dbId != ctx->slot->data.database || -- With Regards, Amit Kapila.

Re: Support logical replication of DDLs

lication can even fail during DDL replication when the function like above is IMMUTABLE and used as follows: ALTER TABLE tbl ADD COLUMN d int DEFAULT t(1); Normally, it is recommended that users can fix such errors by schema-qualifying affected names. See commits 11da97024a and 582edc369c. -- With Regards, Amit Kapila.

Re: DDL result is lost by CREATE DATABASE with WAL_LOG strategy

never understood why that is > outside of the normal numbering space. > Yeah, I have also previously wondered about this name for src/test/subscription/t/100_bugs.pl. My guess is that it has been kept to distinguish it from the other feature tests which have numbering starting from 001. -- With Regards, Amit Kapila.

Re: Change xl_hash_vacuum_one_page.ntuples from int to uint16

On Thu, Feb 16, 2023 at 8:39 PM Drouvot, Bertrand wrote: > > On 2/16/23 1:26 PM, Drouvot, Bertrand wrote: > > Hi, > > > > On 2/16/23 12:00 PM, Amit Kapila wrote: > >> I think this would require XLOG_PAGE_MAGIC as it changes the WAL record. > >> &g

Re: Allow logical replication to copy tables in binary format

esn't. > > > Logical replication between different types like int and smallint is already > not working properly on HEAD too. > Yes, the scenario you shared looks like working. But you didn't create the > subscription with binary=true. The patch did not change subscription with > binary=false case. I believe what you should experiment is binary=true case > which already fails in the apply phase on HEAD. > > Well, with this patch, it will begin to fail in the table copy phase. But I > don't think this is a problem because logical replication in binary format is > already broken for replications between different data types. > So, doesn't this mean that there is no separate failure mode during the initial copy? I am clarifying this to see if the patch really needs a separate copy_format option for initial sync? -- With Regards, Amit Kapila.

Re: Change xl_hash_vacuum_one_page.ntuples from int to uint16

e_page.ntuples from int to uint16. This will create two bytes of padding space in xl_hash_vacuum_one_page which can be used for future patches. This makes the datatype of xl_hash_vacuum_one_page.ntuples same as gistxlogDelete.ntodelete which is advisable as both are used for the same purpose. [1] - https://www.postgresql.org/message-id/2d62f212-fce6-d639-b9eb-2a5bc4bec3b4%40gmail.com -- With Regards, Amit Kapila.

Re: Time delayed LR (WAS Re: logical replication restrictions)

ast patch, I missed that WalSndDelay is actually a subset > of WalSndLoop. Although it can handle keep-alives correctly, I'm not > sure we can accept that structure.. > I think we can use update_progress_txn() for this purpose but note that we are discussing to change the same in thread [1]. [1] - https://www.postgresql.org/message-id/20230210210423.r26ndnfmuifie4f6%40awork3.anarazel.de -- With Regards, Amit Kapila.

Re: Support logical replication of global object commands

On Thu, Feb 16, 2023 at 12:02 PM Amit Kapila wrote: > > On Tue, Aug 30, 2022 at 8:09 AM Zheng Li wrote: > > > > > > I think a publication of ALL OBJECTS sounds intuitive. Does it mean > > > > we'll > > > > publish all DDL commands, all commi

Re: Support logical replication of global object commands

WAL for global objects and then avoid filtering based on the slot's database during decoding. I also thought about whether we want to have a WALSender that is not connected to a database for the replication of global objects but I couldn't come up with a reason for doing so. Do you have any thoughts on this matter? -- With Regards, Amit Kapila.

Re: Support logical replication of DDLs

cess it on the subscriber for each command. -- With Regards, Amit Kapila.

Re: Support logical replication of DDLs

it the > parse node to add the grantor, but I think a better idea might be to > change the signature to > ExecSecLabelStmt(SecLabelStmt *stmt, ObjectAddress *provider) so that > the function can set the provider there; and caller passes > , which is the method we adopted for this kind of thing. > +1, that is a better approach to make the required change in ExecSecLabelStmt(). -- With Regards, Amit Kapila.

Re: Perform streaming logical transactions by background workers and parallel apply

On Wed, Feb 15, 2023 at 8:55 AM houzj.f...@fujitsu.com wrote: > > On Wednesday, February 15, 2023 10:34 AM Amit Kapila > wrote: > > > > > > > > > > So names like the below seem correct format: > > > > > > > > a) WAIT_EVENT_LOGICAL

Re: Support logical replication of DDLs

ted > from node1) will also be sent to node3 causing inconsistency. If the > table has unique or primary key constraints, it will lead to an error. > Can't one use origin = none here to avoid errors? -- With Regards, Amit Kapila.

Re: Support logical replication of DDLs

le for them. > Right, I think we should try a bit harder to avoid adding new CollectedCommandTypes. Is there a reason that we can't retrieve the additional information required to form the command string from system catalogs or parsetree? -- With Regards, Amit Kapila.

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

On Wed, Feb 15, 2023 at 9:23 AM shiy.f...@fujitsu.com wrote: > > On Sat, Feb 4, 2023 7:24 PM Amit Kapila wrote: > > > > On Thu, Feb 2, 2023 at 2:03 PM Önder Kalacı wrote: > > > > > > On v23, I dropped the planner support for picking the index. Instead,

Re: Perform streaming logical transactions by background workers and parallel apply

On Tue, Feb 14, 2023 at 7:45 PM Masahiko Sawada wrote: > > On Tue, Feb 14, 2023 at 3:58 PM Peter Smith wrote: > > > > On Tue, Feb 14, 2023 at 5:04 PM Amit Kapila wrote: > > > > > > On Fri, Feb 10, 2023 at 8:56 AM Peter Smith wrote:

Re: Support logical replication of DDLs

On Fri, Feb 10, 2023 at 4:36 PM Amit Kapila wrote: > > On Thu, Feb 9, 2023 at 3:25 PM Ajin Cherian wrote: > > > > Comments on 0001 and 0002 > === > Few more comments on 0001 and 0003 === 1. I think having 'internal

Re: Perform streaming logical transactions by background workers and parallel apply

On Fri, Feb 10, 2023 at 8:56 AM Peter Smith wrote: > > On Fri, Feb 10, 2023 at 1:32 PM houzj.f...@fujitsu.com > wrote: > > > > On Tuesday, February 7, 2023 11:17 AM Amit Kapila > > wrote: > > > > > > On Mon, Feb 6, 2023 at 3:43 PM houzj.f...@fu

Re: Time delayed LR (WAS Re: logical replication restrictions)

On Fri, Feb 10, 2023 at 4:56 PM Takamichi Osumi (Fujitsu) wrote: > > On Friday, February 10, 2023 2:05 PM Friday, February 10, 2023 2:05 PM wrote: > > On Fri, Feb 10, 2023 at 10:11 AM Amit Kapila > > wrote: > > In the previous patch, we couldn't solve the > timeou

Re: Rework LogicalOutputPluginWriterUpdateProgress

On Sat, Feb 11, 2023 at 2:34 AM Andres Freund wrote: > > On 2023-02-09 11:21:41 +0530, Amit Kapila wrote: > > On Thu, Feb 9, 2023 at 1:33 AM Andres Freund wrote: > > > > > > Hacking on a rough prototype how I think this should rather look, I had a >

Re: Exit walsender before confirming remote flush in logical replication

flush and immedaite for both physical and logical replication. > If we can think of any use case that requires its extension then it makes sense to make it a non-boolean option but otherwise, let's keep things simple by having a boolean option. > If it's > not the case, I don't object to make it a Boolean. > Thanks. -- With Regards, Amit Kapila.

Re: Rework LogicalOutputPluginWriterUpdateProgress

tes before that call which is currently working because pgoutput is tracking the same via sent_begin_txn. Is the intention here that we still track whether BEGIN () has been sent via pgoutput? -- With Regards, Amit Kapila.

<    8   9   10   11   12   13   14   15   16   17   >