On Fri, Oct 23, 2020 at 6:26 PM Ashutosh Bapat
<ashutosh.ba...@2ndquadrant.com> wrote:
>
>
>
> On Fri, 23 Oct 2020 at 18:23, Amit Kapila <amit.kapil...@gmail.com> wrote:
>>
>> On Fri, Oct 23, 2020 at 11:50 AM Kyotaro Horiguchi
>> <horikyota....@gmail.com> wrote:
>> >
>> > At Thu, 22 Oct 2020 22:31:41 -0300, Alvaro Herrera
>> > <alvhe...@alvh.no-ip.org> wrote in
>> > > On 2020-Oct-22, Ashutosh Bapat wrote:
>> > >
>> > > > On Thu, 22 Oct 2020 at 14:46, Kyotaro Horiguchi
>> > > > <horikyota....@gmail.com>
>> > > > wrote:
>> > >
>> > > > > pg_send_logicalrep_msg_type() looks somewhat too-much. If we need
>> > > > > something like that we shouldn't do this refactoring, I think.
>> > > >
>> > > > Enum is an integer, and we want to send byte. The function asserts
>> > > > that the
>> > > > enum fits a byte. If there's a way to declare byte long enums I would
>> > > > use
>> > > > that. But I didn't find a way to do that.
>> > >
>> > > I didn't look at the code, but maybe it's sufficient to add a
>> > > StaticAssert?
>> >
>> > That check needs to visit all symbols in a enum and confirm that each
>> > of them is in a certain range.
>> >
>>
>> Can we define something like LOGICAL_REP_MSG_LAST (also add a comment
>> indicating this is a fake message and must be the last one) as the
>> last and just check that?
>>
>
> I don't think that's required once I applied suggestions from Kyotaro and
> Peter. Please check the latest patch.
> Usually LAST is added to an enum when we need to cap the number of symbols or
> want to find the number of symbols. None of that is necessary here. Do you
> see any other use?
>
You mentioned in the beginning that you prefer to use Enum instead of
define so that switch cases can detect any remaining items but I have
tried adding extra enum value at the end and didn't handle that in
switch case but I didn't get any compilation warning or error. Do we
need something else to detect that at compile time?
Some comments assuming we want to use enum either because I am missing
something or due to some other reason we have not discussed yet.
1.
+ LOGICAL_REP_MSG_STREAM_ABORT = 'A',
+} LogicalRepMsgType;
There is no need for a comma after the last message.
2.
+/*
+ * Logical message types
+ *
+ * Used by logical replication wire protocol.
+ *
+ * Note: though this is an enum it should fit a single byte and should be a
+ * printable character.
+ */
+typedef enum
+{
I think we can expand the comments to probably say why we need these
to fit in a single byte or what problem it can cause if that rule is
disobeyed. This is to make the next person clear why we are imposing
such a rule.
3.
+typedef enum
+{
..
+} LogicalRepMsgType;
There are places in code where we use the enum name
(LogicalRepMsgType) both in the start and end. See TypeCat,
CoercionMethod, CoercionCodes, etc. I see places where we use the way
you have in the code. I would prefer the way we have used at places
like TypeCat as that makes it easier to read.
4.
switch (action)
{
- /* BEGIN */
- case 'B':
+ case LOGICAL_REP_MSG_BEGIN:
apply_handle_begin(s);
- break;
- /* COMMIT */
- case 'C':
+ return;
I think we can simply use 'return apply_handle_begin;' instead of
adding return in another line. Again, I think we changed this handling
in apply_dispatch() to improve the case where we can detect at the
compile time any missing enum but at this stage it is not clear to me
if that is true.
--
With Regards,
Amit Kapila.
