On Wed, May 6, 2026 at 4:55 PM vignesh C <[email protected]> wrote: > > On Fri, 1 May 2026 at 19:16, Dilip Kumar <[email protected]> wrote: > > > > On Thu, Apr 30, 2026 at 10:40 AM Dilip Kumar <[email protected]> wrote: > > > > > > On Wed, Apr 29, 2026 at 12:34 PM shveta malik <[email protected]> > > > wrote: > > > > > > > > On Wed, Apr 29, 2026 at 11:50 AM Dilip Kumar <[email protected]> > > > > wrote: > > > > > > > > > > On Tue, Apr 28, 2026 at 7:53 PM Dilip Kumar <[email protected]> > > > > > wrote: > > > > > > > 2. > > > > > > > +typedef enum ConflictLogDest > > > > > > > +{ > > > > > > > + /* Log conflicts to the server logs */ > > > > > > > + CONFLICT_LOG_DEST_LOG = 1 << 0, /* 0x01 */ > > > > > > > + > > > > > > > + /* Log conflicts to an internally managed conflict log table */ > > > > > > > + CONFLICT_LOG_DEST_TABLE = 1 << 1, /* 0x02 */ > > > > > > > + > > > > > > > + /* Convenience bitmask for all supported destinations */ > > > > > > > + CONFLICT_LOG_DEST_ALL = (CONFLICT_LOG_DEST_LOG | > > > > > > > CONFLICT_LOG_DEST_TABLE) > > > > > > > +} ConflictLogDest; > > > > > > > + > > > > > > > +/* > > > > > > > + * Array mapping for converting internal enum to string. > > > > > > > + */ > > > > > > > +static const char *const ConflictLogDestNames[] = { > > > > > > > + [CONFLICT_LOG_DEST_LOG] = "log", > > > > > > > + [CONFLICT_LOG_DEST_TABLE] = "table", > > > > > > > + [CONFLICT_LOG_DEST_ALL] = "all" > > > > > > > +}; > > > > > > > > > > > > > > Defining an array this way could be an Array size issue. Actually > > > > > > > the > > > > > > > array has just three elements so the last element should be at > > > > > > > ConflictLogDestNames[2] but if we go by the above definition, it > > > > > > > will > > > > > > > be ConflictLogDestNames[3]. Can we define by referring the > > > > > > > following > > > > > > > existing way: > > > > > > > > > > I was analyzing this because I remember we were initially using the > > > > > format you suggested and switched to the bit format to enable direct > > > > > bitwise operations elsewhere. I think Peter suggested that [1], and > > > > > the argument was that the bitwise operation is easy if we represent > > > > > them as a bit. Also, since we would not have too many options, the > > > > > array size shouldn't be an issue. But I understand your point: adding > > > > > more elements will cause the array size to grow very fast as this is > > > > > using sparse array. Let's see what others think about this, and then > > > > > we can decide whether to change it back? > > > > > > > > > > > > > The benefit of the current approach is that checking whether the > > > > destination is TABLE becomes straightforward: > > > > > > > > IsSet(opts.conflictlogdest,CONFLICT_LOG_DEST_TABLE) > > > > > > > > if we go by regular enum values (simialr to XLogSource), then it will > > > > be: > > > > > > > > if (opts.logdest == CONFLICT_LOG_DEST_TABLE || > > > > opts.logdest == CONFLICT_LOG_DEST_ALL) > > > > > > Right > > > > > > > For ease of extending the enum and its corresponding text mappings, my > > > > personal preference is still the regular (non-bitwise) enum approach. > > > > > > Yeah, that's my personal preference too. But Peter had strong stand > > > on keeping as bitwise so that we can directly use > > > IsSet(opts.conflictLogDest, CONFLICT_LOG_DEST_TABLE) operations. > > > Since this array shouldn't have many options, a sparse array is not an > > > issue. So lets see what @Peter Smith has to say here and then we can > > > build a concensus on this. > > > > > > > But if we anticipate adding more destination options in the future > > > > that would be covered by ALL, checking for those in code could lead to > > > > growing chains of OR conditions, whereas the bitwise approach scales > > > > more cleanly in that respect. So I think the choice depends on what > > > > kinds of future extensions we expect. > > > > > > > > Do we have plans to add more options that would naturally fall under > > > > ALL? Or do we instead expect additions that are mutually exclusive; > > > > for example, splitting CONFLICT_LOG_DEST_LOG into something like > > > > CONFLICT_LOG_DEST_JSON_LOG and CONFLICT_LOG_DEST_TEXT_LOG, which may > > > > not make sense to group under ALL in the same way? > > > > > > Currently, I haven't considered which options would naturally fall > > > under "ALL." Perhaps if we plan targets other than logs and files, > > > those might also fall under "ALL." > > > > I have fixed all the reported comments except these four. > > Few minor comments: > 1) Now that we create the table in pg_conflict system schema where > other users cannot create the table, is there a scenario where this is > possible? > /* > * Check for an existing table with the sname name in the > pg_conflict namespace. > * A collision should not occur under normal operation, but we > must handle cases > * where a table has been created manually. > */ > if (OidIsValid(get_relname_relid(relname, PG_CONFLICT_NAMESPACE))) > ereport(ERROR, > (errcode(ERRCODE_DUPLICATE_TABLE), > errmsg("conflict log table pg_conflict.\"%s\" already > exists", relname), > errhint("A table with the same name already exists. " > "To proceed, drop the existing table and retry."))); >
It is possible to hit it with allow_system_table_mods=on. See issue1 raised by Nisha in [1] [1]: https://www.postgresql.org/message-id/CABdArM6jpLnzC5O%3DX48RpFXRmAr5WOSHJtw0ebT%2B7Wmb-WdfvQ%40mail.gmail.com thanks Shveta
