Hi David, The feature seems useful to me. The code will need to be refactored due to changes in commit : b05fe7b442
Please see the following comments. 1. Is there a specific reason behind having new relstate for truncate? The current state flow is INIT->DATATSYNC->SYNCWAIT->CATCHUP->SYNCDONE->READY. Can we accommodate the truncate in either INIT or DATASYNC? 2. + StartTransactionCommand(); + rel = table_open(MyLogicalRepWorker->relid, RowExclusiveLock); + + rels = lappend(rels, rel); + relids = lappend_oid(relids, MyLogicalRepWorker->relid); + + ExecuteTruncateGuts(rels, relids, NIL, DROP_RESTRICT, false); + CommitTransactionCommand(); Truncate is being performed in a separate transaction as data copy, I think that leaves a window open for concurrent transactions to modify the data after truncate and before copy. 3. Regarding the truncate of the referenced table, I think one approach can be to perform the following: i. lock the referencing and referenced tables against writes ii. drop the foriegn key constraints, iii.truncate iv. sync v. recreate the constraints vi. release lock. However, I am not sure of the implications of locking these tables on the main apply process. Thank you, On Mon, Oct 12, 2020 at 11:31 PM David Christensen <da...@endpoint.com> wrote: > > On Oct 11, 2020, at 10:00 PM, Amit Kapila <amit.kapil...@gmail.com> > wrote: > > > > On Mon, Oct 12, 2020 at 3:44 AM David Christensen <da...@endpoint.com> > wrote: > >> > >> > >> On Oct 11, 2020, at 1:14 PM, Euler Taveira < > euler.tave...@2ndquadrant.com> wrote: > >> > >> > >> On Fri, 9 Oct 2020 at 15:54, David Christensen <da...@endpoint.com> > wrote: > >>> > >>> > >>> Enclosed find a patch to add a “truncate” option to subscription > commands. > >>> > >>> When adding new tables to a subscription (either via `CREATE > SUBSCRIPTION` or `REFRESH PUBLICATION`), tables on the target which are > being newly subscribed will be truncated before the data copy step. This > saves explicit coordination of a manual `TRUNCATE` on the target tables and > allows the results of the initial data sync to be the same as on the > publisher at the time of sync. > >>> > >>> To preserve compatibility with existing behavior, the default value > for this parameter is `false`. > >>> > >> > >> Truncate will fail for tables whose foreign keys refer to it. If such a > feature cannot handle foreign keys, the usefulness will be restricted. > >> > >> > >> This is true for existing “truncate” with FKs, so doesn’t seem to be > any different to me. > >> > > > > What would happen if there are multiple tables and truncate on only > > one of them failed due to FK check? Does it give an error in such a > > case, if so will the other tables be truncated? > > Currently each SyncRep relation is sync’d separately in its own worker > process; we are doing the truncate at the initialization step of this, so > it’s inherently in its own transaction. I think if we are going to do any > sort of validation on this, it would have to be at the point of the CREATE > SUBSCRIPTION/REFRESH PUBLICATION where we have the relation list and can do > sanity-checking there. > > Obviously if someone changes the schema at some point between when it does > this and when relation syncs start there is a race condition, but the same > issue would affect other data sync things, so I don’t care to solve that as > part of this patch. > > >> Hypothetically if you checked all new tables and could verify if there > were FK cycles only already in the new tables being added then “truncate > cascade” would be fine. Arguably if they had existing tables that were part > of an FK that wasn’t fully replicated they were already operating brokenly. > >> > > > > I think if both PK_table and FK_table are part of the same > > subscription then there should be a problem as both them first get > > truncated? If they are part of a different subscription (or if they > > are not subscribed due to whatever reason) then probably we need to > > deal such cases carefully. > > You mean “should not be a problem” here? If so, I agree with that. > Obviously if we determine this features is only useful with this support > we’d have to chase the entire dependency graph and make sure that is all > contained in the set of newly-subscribed tables (or at least FK referents). > > I have not considered tables that are part of more than one subscription > (is that possible?); we presumably should error out if any table exists > already in a separate subscription, as we’d want to avoid truncating tables > already part of an existing subscription. > > While I’m happy to take a stab at fixing some of the FK/PK issues, it > seems easy to go down a rabbit hole. I’m not convinced that we couldn’t > just detect FK issues and choose to not handle this case without decreasing > the utility for at least some cases. (Perhaps we could give a hint as to > the issues detected to point someone in the right direction.) Anyway, glad > to keep discussing potential implications, etc. > > Best, > > David > -- > David Christensen > Senior Software and Database Engineer > End Point Corporation > da...@endpoint.com > 785-727-1171 > > > > >