Re: Include sequence relation support in logical replication - Mailing list pgsql-hackers

Hi,

On 2020-03-24 16:19:21 -0700, Cary Huang wrote:
> I have shared a patch that allows sequence relation to be supported in
> logical replication via the decoding plugin ( test_decoding for
> example ); it does not support sequence relation in logical
> replication between a PG publisher and a PG subscriber via pgoutput
> plugin as it will require much more work.

Could you expand on "much more work"? Once decoding support is there,
that shouldn't be that much?


> Sequence changes caused by other sequence-related SQL functions like
> setval() or ALTER SEQUENCE xxx, will always emit a WAL update, so
> replicating changes caused by these should not be a problem.

I think this really would need to handle at the very least setval to
make sense.


> For the replication to make sense, the patch actually disables the WAL
> update at every 32 nextval() calls, so every call to nextval() will
> emit a WAL update for proper replication. This is done by setting
> SEQ_LOG_VALS to 0 in sequence.c

Why is that needed? ISTM updating in increments of 32 is fine for
replication purposes? It's good imo, because sending out more granular
increments would increase the size of the WAL stream?



> diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c
> index 93c948856e..7a7e572d6c 100644
> --- a/contrib/test_decoding/test_decoding.c
> +++ b/contrib/test_decoding/test_decoding.c
> @@ -466,6 +466,15 @@ pg_decode_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn,
>                                     &change->data.tp.oldtuple->tuple,
>                                     true);
>             break;
> +        case REORDER_BUFFER_CHANGE_SEQUENCE:
> +                    appendStringInfoString(ctx->out, " SEQUENCE:");
> +                    if (change->data.sequence.newtuple == NULL)
> +                        appendStringInfoString(ctx->out, " (no-tuple-data)");
> +                    else
> +                        tuple_to_stringinfo(ctx->out, tupdesc,
> +                                            &change->data.sequence.newtuple->tuple,
> +                                            false);
> +                    break;
>         default:
>             Assert(false);
>     }

You should also add tests - the main purpose of contrib/test_decoding is
to facilitate writing those...


> +    ReorderBufferXidSetCatalogChanges(ctx->reorder, XLogRecGetXid(buf->record), buf->origptr);

Huh, why are you doing this? That's going to increase overhead of logical
decoding by many times?


> +                case REORDER_BUFFER_CHANGE_SEQUENCE:
> +                    Assert(snapshot_now);
> +
> +                    reloid = RelidByRelfilenode(change->data.sequence.relnode.spcNode,
> +                                                change->data.sequence.relnode.relNode);
> +
> +                    if (reloid == InvalidOid &&
> +                        change->data.sequence.newtuple == NULL)
> +                        goto change_done;

I don't think this path should be needed? There's afaict no valid ways
we should be able to end up here without a tuple?


> +                    if (!RelationIsLogicallyLogged(relation))
> +                        goto change_done;

Similarly, this seems superflous and should perhaps be an assertion?

> +                    /* user-triggered change */
> +                    if (!IsToastRelation(relation))
> +                    {
> +                        ReorderBufferToastReplace(rb, txn, relation, change);
> +                        rb->apply_change(rb, txn, relation, change);
> +                    }
> +                    break;
>             }
>         }
>

This doesn't make sense either.



> diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
> index 626ecf4dc9..cf3fd45c5f 100644
> --- a/src/include/replication/reorderbuffer.h
> +++ b/src/include/replication/reorderbuffer.h
> @@ -62,7 +62,8 @@ enum ReorderBufferChangeType
>     REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID,
>     REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT,
>     REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM,
> -    REORDER_BUFFER_CHANGE_TRUNCATE
> +    REORDER_BUFFER_CHANGE_TRUNCATE,
> +    REORDER_BUFFER_CHANGE_SEQUENCE,
> };
>
> /* forward declaration */
> @@ -149,6 +150,15 @@ typedef struct ReorderBufferChange
>             CommandId    cmax;
>             CommandId    combocid;
>         }            tuplecid;
> +        /*
> +         * Truncate data for REORDER_BUFFER_CHANGE_SEQUENCE representing one
> +         * set of relations to be truncated.
> +         */

What?

> +        struct
> +        {
> +            RelFileNode relnode;
> +            ReorderBufferTupleBuf *newtuple;
> +        }            sequence;
>     }            data;
>
>     /*

I don't think we should expose sequence changes via their tuples -
that'll unnecessarily expose a lot of implementation details.

Greetings,

Andres Freund