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." -- Regards, Dilip Kumar Google
