Re: [pgAdmin][RM5912]: Added support for Logical Replication.

Thanks, the patch applied with some fixes.

On Wed, Jan 27, 2021 at 11:09 AM Pradip Parkale <
pradip(dot)parkale(at)enterprisedb(dot)com> wrote:

> Hi Akshay,
> Please find the updated patch. I have fixed almost all issues, some are
> not possible.
>
> On Sat, Jan 16, 2021 at 4:29 PM Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> Hi Pradip
>>
>> Following are the GUI related review comments:
>>
>> *Publication:*
>>
>> 1. The truncate option is not visible for PG 10 but the SQL query
>> showing it.
>>
>> Fixed,
>
>>
>> 1. "All table?" should be "All tables?" and "Table" should be renamed
>> to "Tables".
>>
>> Fixed.
>
>>
>> 1. "-- Publication : test;
>> -- DROP PUBLICATION test;" Follow the syntax(Spacing, no space
>> between colon) as we did for other nodes. Check the SQL tab for any other
>> node.
>>
>> Fixed.
>
>>
>> 1. Select any publication and open the properties dialog, Save button
>> is enabled by default which should not.
>>
>> Fixed.
>
>>
>> 1. Create publication with (insert, update, delete, and truncate).
>> Open the properties dialog and Set the value of Insert, Update and Delete
>> to "No" check the SQL tab no ALTER query is there. ALTER query should be
>> there.
>>
>> Fixed.
>
>>
>> 1. 'Only Table' is not visible in the Properties panel.
>>
>> There is no entry of 'Only Table?' in the pg_publication table, so it is
> not possible to get the value of this.
>
>>
>> 1. On the collection node 'Publications' we should add Tables in the
>> properties panel.
>>
>> Fixed.
>
>>
>> 1. Create a publication using one table and set 'Only Table' to No.
>> Select a newly created publication and set 'Only Table' to Yes, no SQL
>> query is generated.
>>
>> I have disabled this control. This control will be enabled only when the
> user has changed the table data.
>
>>
>> 1. Adding a table in the publication should generate 'ALTER
>> PUBLICATION .. ADD TABLE ..' syntax instead of 'ALTER PUBLICATION .. SET
>> TABLE..'
>>
>> Fixed.
>
>>
>> 1. Create a publication with more than one table. Remove one table
>> from publication it is creating two ALTER statements one for DROP Table and
>> another one is to SET TABLE which I think not required.
>>
>> Fixed.
>
>> *Subscription:*
>>
>> 1. Reduce the width of the subscription dialog.
>>
>> Fixed. I reduced the width to '501px'
>
>>
>> 1. *With *tab, alignment is not proper. Switch control should be
>> aligned with others.
>>
>> Fixed.
>
>>
>> 1. Help messages required for every control in *With* tab.
>>
>> Fixed.
>
>>
>> 1. Open Create subscription dialog, specify the name, on the
>> connection tab click the button to get the publication without specifying
>> any details. One popup comes with msg 'host'. I would suggest to
>> *disabled* that button until all the required fields will be
>> populated or entered by the user.
>>
>> Fixed.' Publication' control will only be enabled when the user
> entered the required connection details.
>
>>
>> 1. Publication list from the same database server should be listed by
>> default.
>>
>> This won't be a good idea to display the default publication because if
> the user entered the connection details of some other server and select the
> default publication without clicking on the 'get_publication' button then
> it will be a wrong mapping between connection details and publication.
> Logical replication won't happen then.
>
>>
>> 1. Specify all the required connection details on the connection tab
>> and click on the 'get publication' button. It fetches the publication but
>> no message for the user that action is successfully completed. For the
>> user, it seems nothing is happening and the user will click on that button
>> continuously. *Suggestion: *Show some message 'Publications fetched
>> successfully.' or something similar.
>>
>> Fixed. Added information message at the right bottom of the page.
>
>>
>> 1. "-- Subscription : my_sub; -- DROP SUBSCRIPTION my_sub;" Follow
>> the syntax(Spacing, no space between colon) as we did for other nodes.
>> Check the SQL tab for any other node.
>>
>> Done.
>
>>
>> 1. Remove the unnecessary spaces from the following SQL:
>> - CONNECTION ' host=127.0.0.1 port=5436 user=postgres
>> dbname=postgres '
>> - with (enabled = False, slot_name = my_sub1, synchronous_commit
>> = 'off');
>> - (connect = True, enabled = True, copy_data = True, create_slot =
>> True, synchronous_commit = 'off'); Use small true/false instead of
>> True/False.
>>
>> Fixed.
>
>>
>> 1. Create a subscription. Open the properties dialog and click on the
>> "get publication" button. It throws an error message with the string
>> 'password', please provide a valid error message. Check for all such error
>> messages.
>>
>> Fixed.
>
>>
>> 1. Consider the above(Point 9) scenario and provide a password and
>> then click on the "get publication" button it throws an error "*could
>> not translate host name "127.0.0.1 " to address: nodename nor servname
>> provided, or not known*". There is a space after the IP address, even
>> if we remove that space or provide a different IP address the same error
>> occurs.
>>
>> Fixed.
>
>>
>> 1. The "Refresh publication" option should be disabled if the
>> subscription is disabled, as it should not work with the disabled
>> subscription.
>>
>> Fixed.
>
>>
>> 1. No SQL query created when we remove the Slot name for the existing
>> subscription. How to handle:
>> - If the subscription is enabled then add validation "Slot name
>> can not be empty".
>> - If the subscription is disabled then on removing the slot name
>> should create "ALTER SUBSCRIPTION <name> SET (slot_name = NONE);"
>>
>> Fixed. On removing the slot name I'm adding setting the slot name as
> 'None'.
>
>>
>> 1. Provide a space after comma in the current publication list.
>>
>> Fixed.
>
>> Please explain how the service file parameter will work with the "CREATE
>> SUBSCRIPTION .." syntax as per documentation we have to provide the
>> connection info string. Not able to test it.
>>
> Removed the service parameter as it is not useful in the connection
> string.
>
> I have also fixed the review comments given in the demo. Subscription
> doesn't maintain any 'dependency' and 'dependent'. So I have not added that.
>
>>
>> Created #6153 <https://redmine.postgresql.org/issues/6153> to add
>> publication and subscription in Schema Diff.
>>
>> On Thu, Jan 14, 2021 at 6:58 PM Pradip Parkale <
>> pradip(dot)parkale(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Akshay,
>>> Please find the latest patch. I missed some files in my previous patch.
>>>
>>> On Thu, Jan 14, 2021 at 5:49 PM Akshay Joshi <
>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Pradip
>>>>
>>>> The patch is applied, but not working. Please check and send the patch
>>>> again.
>>>>
>>>> On Wed, Jan 13, 2021 at 9:08 AM Pradip Parkale <
>>>> pradip(dot)parkale(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Akshay,
>>>>>
>>>>> Please ignore my previous patch. Please find my updated patch.
>>>>>
>>>>>
>>>>> On Mon, Jan 11, 2021 at 5:07 PM Pradip Parkale <
>>>>> pradip(dot)parkale(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi Hackers,
>>>>>>
>>>>>> Please find the attached patch for logical replication support.
>>>>>>
>>>>>> --
>>>>>> Thanks & Regards,
>>>>>> Pradip Parkale
>>>>>> Software Engineer | EnterpriseDB Corporation
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Thanks & Regards,
>>>>> Pradip Parkale
>>>>> Software Engineer | EnterpriseDB Corporation
>>>>>
>>>>
>>>>
>>>> --
>>>> *Thanks & Regards*
>>>> *Akshay Joshi*
>>>> *pgAdmin Hacker | Principal Software Architect*
>>>> *EDB Postgres <http://edbpostgres.com>*
>>>>
>>>> *Mobile: +91 976-788-8246*
>>>>
>>>
>>>
>>> --
>>> Thanks & Regards,
>>> Pradip Parkale
>>> Software Engineer | EnterpriseDB Corporation
>>>
>>
>>
>> --
>> *Thanks & Regards*
>> *Akshay Joshi*
>> *pgAdmin Hacker | Principal Software Architect*
>> *EDB Postgres <http://edbpostgres.com>*
>>
>> *Mobile: +91 976-788-8246*
>>
>
>
> --
> Thanks & Regards,
> Pradip Parkale
> Software Engineer | EnterpriseDB Corporation
>

--
*Thanks & Regards*
*Akshay Joshi*
*pgAdmin Hacker | Principal Software Architect*
*EDB Postgres <http://edbpostgres.com>*

*Mobile: +91 976-788-8246*