On Mon, 6 Apr 2026 at 15:08, Alvaro Herrera <[email protected]> wrote: > > On 2026-Apr-06, vignesh C wrote: > > > On Mon, 6 Apr 2026 at 02:12, Alvaro Herrera <[email protected]> wrote: > > > Few comments: > > Thanks for reviewing this patch! > > > 1) Can we add a comment why we should error out here, as repack > > concurrently requires this whereas repack does not require this check. > > Even if it is required for decoding can't it be handled by replica > > identity full: > > + /* > > + * If the identity index is not set due to replica identity being, > > PK > > + * might exist. > > + */ > > + ident_idx = RelationGetReplicaIndex(rel); > > + if (!OidIsValid(ident_idx) && OidIsValid(rel->rd_pkindex)) > > + ident_idx = rel->rd_pkindex; > > + if (!OidIsValid(ident_idx)) > > + ereport(ERROR, > > + > > errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > > + errmsg("cannot process relation \"%s\"", > > + RelationGetRelationName(rel)), > > + errhint("Relation \"%s\" has no > > identity index.", > > + > > RelationGetRelationName(rel))); > > Ah, I was just rewriting that comment moments ago. I think we could > make it work with replica identity full in theory, but I'm guessing it > would be useless. You really need an index that lets you locate the > affected tuples; otherwise the replay would take forever. If the table > is big, that would make the whole thing unworkable. If the table isn't > big, then you can probably just use straight repack without too much > disruption. > > /* > * Obtain the replica identity index -- either one that has been set > * explicitly, or the primary key. If none of these cases apply, the > * table cannot be repacked concurrently. It might be possible to > have > * repack work with a FULL replica identity; however that requires > more > * work and is not implemented yet. > */
Should this be mentioned in XXX comment: It might be possible to have repack work with a FULL replica identity; however that requires more work and is not implemented yet. > > 2) Do you think it will be good to add a test to simulate a case where > > one of the swap_replation_files is successful and a failure after > > that. We can verify that the oid should still point to old oids: > > Hmm, it's not clear to me in which cases this can happen. Are you > thinking that the first swap_replation_files call dies because of > out-of-memory? Yes, I was thinking of that. > Note that the really weird cases, like pg_class or mapped relations, are > directly rejected. So we don't get into the branch with > !RelFileNumberIsValid, and so on. > > I mean -- I'm not opposed to adding a test case for it. But I suspect > it's going to be somewhat annoying to write. I will verify this scenario through debugger. Regards, Vignesh
