Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1l6Vm2-0006cG-DC for pgadmin-hackers@arkaria.postgresql.org; Mon, 01 Feb 2021 09:45:34 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1l6Vm0-00076n-Ty for pgadmin-hackers@arkaria.postgresql.org; Mon, 01 Feb 2021 09:45:32 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1l6Vm0-00076f-B7 for pgadmin-hackers@lists.postgresql.org; Mon, 01 Feb 2021 09:45:32 +0000 Received: from mail-il1-x131.google.com ([2607:f8b0:4864:20::131]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1l6Vld-0002St-2s for pgadmin-hackers@postgresql.org; Mon, 01 Feb 2021 09:45:31 +0000 Received: by mail-il1-x131.google.com with SMTP id y5so14946457ilg.4 for ; Mon, 01 Feb 2021 01:45:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=VYfH5hNYD+Nu/odQNocah1f8yuErqVtVRWw+culpfHE=; b=0TjYTMSoTk+v+FJSjavOpxzU1f/7Pv1xRbsklLg4Pj9yUmkwgC7y9Pzn/Hopl9zLrP ANnb9++ffZLVE47CN0r8SnNb3/1b0OcVRgHEtA2NaqJ4VgLm37X2frwhfCR2nD69+8Zv KiYm8AKmh5wQFTYVqcNsEmgQG2Ch5fkrlFNFl+tIOmbFeR6cwKOM0oCHwGCAFEBc/BGT wZX94bA3fPuAdU/tAIlbTrc4o6LjSnZtEwdvaVQD1xN53VxasowPCbqu8yUkn34yXG9N kyAvjncQGQ8SQHMvr8ihVYG6uy5cZaDJUbOCdXwVtZDJk35hNZKUu12tc5RkDPecw4Pj CE6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VYfH5hNYD+Nu/odQNocah1f8yuErqVtVRWw+culpfHE=; b=GPPTYKbww5pnB59rUwpPaacWdpfkw6TJDk77VVEt8ggYhTjlwd/3O1RdrJY7Hbg2WW vXHbPtn+7/Qv8QSr2uCt3mPBjkZpT4jdzY/frCe8tgDql/TJc9huR+V3yynyipsC2wuV bKN69BCtQeB8V/2NwEiyADJLOFpPF38k8wRgGO3zfQhQynfosJBPQoZrns4l3nLgwV9A nvu/qroatyYxvxeJQJqKdrDVpkjplg7QqcE2OjnA1hHbn6kDq+zYVPp9ZcgD/bvlhINA nNEcAvZMa++c3S4izarmcRYpfjTpHWUS8unQ+ggHVHw4sJCD82yBlbc3L2YXOKVBYBOv FIaA== X-Gm-Message-State: AOAM530N1aoZ9mjXRnjQ+/AskklOapPKSRntilOGdLLJ8bnXUWinkhX7 DBMzqSvVSPAdZsGJI5r5hcmC44fleLM9Z3lr8E60/otSkWNxusRvXNUBlELjixH6UGRxayvqZkC eXElS8paigDaWWI87lZVEusQsNwQgfpJLlxfEqEsvakefuynkWV0QwJ8ZBKx7QhnrxN+PRRwrD1 LzmYqbdGQVVy+MNFd11+R4uvRt0Sk/yJkGkyvdKKpmi/w+qIxyManTuORbinXg1eIy0g== X-Google-Smtp-Source: ABdhPJw4PvcWX5HBHAF4j3FRm9i1zJkGx6eV07Vh8mLJIXXXEofy5wMClhy8FzxwxeFfGzspPu3SgbC9GIG/vr23I+s= X-Received: by 2002:a05:6e02:1985:: with SMTP id g5mr11793510ilf.257.1612172707967; Mon, 01 Feb 2021 01:45:07 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Akshay Joshi Date: Mon, 1 Feb 2021 15:14:56 +0530 Message-ID: Subject: Re: [pgAdmin][RM5912]: Added support for Logical Replication. To: Pradip Parkale Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000934eb705ba433599" X-CLOUD-SEC-AV-Info: enterprisedb,google_mail,monitor X-CLOUD-SEC-AV-Sent: true X-Gm-Spam: 0 X-Gm-Phishy: 0 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000934eb705ba433599 Content-Type: text/plain; charset="UTF-8" Thanks, the patch applied with some fixes. On Wed, Jan 27, 2021 at 11:09 AM Pradip Parkale < pradip.parkale@enterprisedb.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.joshi@enterprisedb.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 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 to add >> publication and subscription in Schema Diff. >> >> On Thu, Jan 14, 2021 at 6:58 PM Pradip Parkale < >> pradip.parkale@enterprisedb.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.joshi@enterprisedb.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.parkale@enterprisedb.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.parkale@enterprisedb.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 * >>>> >>>> *Mobile: +91 976-788-8246* >>>> >>> >>> >>> -- >>> Thanks & Regards, >>> Pradip Parkale >>> Software Engineer | EnterpriseDB Corporation >>> >> >> >> -- >> *Thanks & Regards* >> *Akshay Joshi* >> *pgAdmin Hacker | Principal Software Architect* >> *EDB Postgres * >> >> *Mobile: +91 976-788-8246* >> > > > -- > Thanks & Regards, > Pradip Parkale > Software Engineer | EnterpriseDB Corporation > -- *Thanks & Regards* *Akshay Joshi* *pgAdmin Hacker | Principal Software Architect* *EDB Postgres * *Mobile: +91 976-788-8246* --000000000000934eb705ba433599 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks, the patch applied with some fixes.

On Wed, Jan 27, = 2021 at 11:09 AM Pradip Parkale <pradip.parkale@enterprisedb.com> wrote:
Hi Akshay,
Please find the updated patch. I have fixed almost all iss= ues, some are not possible.

On Sat, Jan 16, 2021 at 4:29 PM Akshay Jos= hi <a= kshay.joshi@enterprisedb.com> wrote:
Hi Pradip

F= ollowing are the GUI related review comments:

Publication:
  1. The truncate option is not visible= for PG 10 but the SQL query showing it.
=
Fixed,=C2=A0
  1. "All table?" should be "All tabl= es?" and "Table" should be renamed to "Tables".
Fixed.=C2=A0
  1. "-- Public= ation : test;
    -- DROP PUBLICATION test;" Follow the syntax(Spacing,= no space between colon) as we did for other nodes. Check the SQL tab for a= ny other node.
Fixed.=C2=A0
  • Select any publication and open the properties dialog, Save button is= enabled by default which should not.
    • Fixed.=C2=A0
    1. Create publication with (insert, update, delete, a= nd truncate). Open the properties dialog and Set the value of Insert, Updat= e and Delete to "No" check the SQL tab no ALTER query is there. A= LTER query should be there.
    Fixed.= =C2=A0
    1. 'Only Table' is not visible in the Properties pane= l.
    There is no entry of 'Only Ta= ble?' in the pg_publication table, so it is not possible to get the val= ue of this.=C2=A0
    1. On the collection node 'Publications' w= e should add Tables in the properties panel.
    Fixed.=C2=A0
    1. Create a publication using one table and se= t 'Only Table' to No. Select a newly created publication and set &#= 39;Only Table' to Yes, no SQL query is generated.
    =
    I have disabled this control. This control will be enable= d only when the user has changed the table data.=C2=A0
    1. Adding a t= able in the publication should generate 'ALTER PUBLICATION .. ADD TABLE= ..' syntax instead of 'ALTER PUBLICATION .. SET TABLE..'
      2. <= /ol>
    Fixed.=C2=A0
    1. Create a publicati= on with more than one table. Remove one table from publication it is creati= ng=C2=A0two ALTER statements one for DROP Table and another one is to SET T= ABLE which I think not required.
    Fixed.=C2=A0
    Subscription:
    1. Reduce the width of = the subscription dialog.
    Fixed. I re= duced the width to '501px'=C2=A0
    1. With tab, alignme= nt is not proper. Switch control should be aligned with others.
    Fixed.=C2=A0
    1. Help messages required f= or every control in With=C2=A0tab.
    Fixed.=C2=A0
    <= div dir=3D"ltr">
    1. Open Create subscription dialog, specify the n= ame, on the connection tab click the button to get the publication without = specifying any details. One popup comes with msg 'host'. I would su= ggest to disabled that button until all the required fields will be = populated or entered by the user.
    Fi= xed.' Publication' control will only be enabled when the user enter= ed=C2=A0the required connection details.=C2=A0
    1. Publication list f= rom 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 othe= r server and select the default publication without clicking on the 'ge= t_publication' button then it will be a wrong mapping between connectio= n details and publication. Logical replication won't happen then.
    =
  • Specify all the required connection details on the connection tab and = click on the 'get publication' button. It fetches the publication b= ut no message for the user that action is successfully completed. For the u= ser, it seems nothing is happening and the user will click on that button c= ontinuously. Suggestion: Show some message 'Publications fetched= successfully.' or something similar.
    • Fixed. Added information message at the right bottom of the page.
    <= ol>
  • "-- Subscription : my_sub; -- DROP SUBSCRIPTION my_sub;"= =C2=A0Follow the syntax(Spacing, no space between colon) as we did for othe= r nodes. Check the SQL tab for any other node.
    • Done.=C2=A0
    1. Remove the unnecessary=C2=A0spaces from th= e following SQL:
      • CONNECTION ' host=3D127.0.0.1 =C2=A0port= =3D5436 user=3Dpostgres dbname=3Dpostgres '
      • with (enabled =3D F= alse, =C2=A0slot_name =3D my_sub1, synchronous_commit =3D 'off');
      • (connect =3D True, enabled =3D True, copy_data =3D True, create_slot= =3D True, synchronous_commit =3D 'off');=C2=A0 Use small true/fals= e instead of True/False.
    Fixed.= =C2=A0
    1. Create a subscription. Open the properties dialog and clic= k on the "get publication" button. It throws an error message wit= h the string 'password', please provide a valid error message. Chec= k for all such error messages.
    Fixed= .=C2=A0
    1. Consider the above(Point 9) scenario and provide a passwo= rd and then click on the "get publication" button it throws an er= ror "could not translate host name &quo= t;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 s= pace or provide a different IP address the same error occurs.
    Fixed.=C2=A0
    1. The "Refresh publication" option should be disabled if t= he subscription is disabled, as it should not work with the disabled subscr= iption.
    Fixed.=C2=A0
      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 &= quot;Slot name can not be empty".
      • If the subscription is disabled then on removing the slot name shoul= d create "ALTER SUBSCRIPTION <name> SET (slot_name =3D NO= NE);"
    Fixed. On removing t= he slot name I'm adding setting the slot name as 'None'.=C2=A0<= /div>
    1. Provide a space after comma in the current publication list.
      2. =
    Fixed.=C2=A0
    Please explain how t= he service file parameter will work=C2=A0with the "CREATE SUBSC= RIPTION .." syntax as per documentation we have to provide the connect= ion info string. Not able to test it.
    Removed the service parameter as it is not useful in the connection strin= g.=C2=A0

    I have also fixed the review comments giv= en in the demo. Subscription doesn't maintain any 'dependency' = and 'dependent'. So I have not added that.

    Created=C2=A0#6153=C2=A0to add publication and subscripti= on in Schema Diff.

    On Thu, Jan 14, 2021 at 6:58 PM Pradip = Parkale <pradip.parkale@enterprisedb.com> wrote:
    Hi=C2=A0Akshay,
    P= lease find the latest patch. I missed some files in my previous patch.

    On Thu, Jan 14, 2021 at 5:49 PM Akshay Joshi <akshay.joshi@enterprisedb.com= > wrote:
    Hi=C2=A0Pradip

    The patch is applied, but no= t working. Please check and send the patch again.

    On Wed, Jan 13, 2021= at 9:08 AM Pradip Parkale <pradip.parkale@enterprisedb.com> wrote:
    =
    Hi= Akshay,

    Please ignore=C2=A0my=C2=A0previous patch. Plea= se find my updated patch.


    On Mon, Jan 11, 2021 at 5= :07 PM Pradip Parkale <pradip.parkale@enterprisedb.com> wrote:
    Hi Hack= ers,

    Please find the attached patch for logical replicat= ion support.

    --
    Thanks & Regards,
    <= div style=3D"font-size:12.8px">Pradip Parkale
    Software Engineer | Enterp= riseDB Corporation


    --
    Thanks & Regards,
    =
    Pradip Parkale
    Software Engineer |= EnterpriseDB Corporation


    --
    Thank= s & Regards
    Akshay Joshi
    pgAdmin Hacker | Principal Softw= are Architect
    EDB Po= stgres
    Mobile: +91 976-788-8246



    --
    Thanks & Regards,
    =
    Pradip Parkale
    Software Engineer |= EnterpriseDB Corporation


    --
    Thank= s & Regards
    Akshay Joshi
    pgAdmin Hacker | Principal Softw= are Architect
    EDB Po= stgres
    Mobile: +91 976-788-8246



    --
    Thanks & Regards,
    =
    Pradip Parkale
    Software Engineer |= EnterpriseDB Corporation


    --
    Thanks & Regards
    Akshay Joshi
    pgAdmi= n Hacker | Principal Software Architect
    EDB Postgres
    Mobile: +91 976-788-8246

    --000000000000934eb705ba433599--