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

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