Re: bogus: logical replication rows/cols combinations

On 5/9/22 05:45, Amit Kapila wrote:
> On Sun, May 8, 2022 at 11:41 PM Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>>
>> On 5/7/22 07:36, Amit Kapila wrote:
>>> On Mon, May 2, 2022 at 6:11 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>>>>
>>>> On 2022-May-02, Amit Kapila wrote:
>>>>
>>>>
>>>>> I think it is possible to expose a list of publications for each
>>>>> walsender as it is stored in each walsenders
>>>>> LogicalDecodingContext->output_plugin_private. AFAIK, each walsender
>>>>> can have one such LogicalDecodingContext and we can probably share it
>>>>> via shared memory?
>>>>
>>>> I guess we need to create a DSM each time a walsender opens a
>>>> connection, at START_REPLICATION time. Then ALTER PUBLICATION needs to
>>>> connect to all DSMs of all running walsenders and see if they are
>>>> reading from it. Is that what you have in mind? Alternatively, we
>>>> could have one DSM per publication with a PID array of all walsenders
>>>> that are sending it (each walsender needs to add its PID as it starts).
>>>> The latter might be better.
>>>>
>>>
>>> While thinking about using DSM here, I came across one of your commits
>>> f2f9fcb303 which seems to indicate that it is not a good idea to rely
>>> on it but I think you have changed dynamic shared memory to fixed
>>> shared memory usage because that was more suitable rather than DSM is
>>> not portable. Because I see a commit bcbd940806 where we have removed
>>> the 'none' option of dynamic_shared_memory_type. So, I think it should
>>> be okay to use DSM in this context. What do you think?
>>>
>>
>> Why would any of this be needed?
>>
>> ALTER PUBLICATION will invalidate the RelationSyncEntry entries in all
>> walsenders, no? So AFAICS it should be enough to enforce the limitations
>> in get_rel_sync_entry,
>>
>
> Yes, that should be sufficient to enforce limitations in
> get_rel_sync_entry() but it will lead to the following behavior:
> a. The Alter Publication command will be successful but later in the
> logs, the error will be logged and the user needs to check it and take
> appropriate action. Till that time the walsender will be in an error
> loop which means it will restart and again lead to the same error till
> the user takes some action.
> b. As we use historic snapshots, so even after the user takes action
> say by changing publication, it won't be reflected. So, the option for
> the user would be to drop their subscription.
>
> Am, I missing something? If not, then are we okay with such behavior?
> If yes, then I think it would be much easier implementation-wise and
> probably advisable at this point. We can document it so that users are
> careful and can take necessary action if they get into such a
> situation. Any way we can improve this in future as you also suggested
> earlier.
>
>> which is necessary anyway because the subscriber
>> may not be connected when ALTER PUBLICATION gets executed.
>>
>
> If we are not okay with the resultant behavior of detecting this in
> get_rel_sync_entry(), then we can solve this in some other way as
> Alvaro has indicated in one of his responses which is to detect that
> at start replication time probably in the subscriber-side.
>

IMO that behavior is acceptable. We have to do that check anyway, and
the subscription may start failing after ALTER PUBLICATION for a number
of other reasons anyway so the user needs/should check the logs.

And if needed, we can improve this and start doing the proactive-checks
during ALTER PUBLICATION too.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company