From: Xiong W. <wan...@gm...> - 2013-02-28 02:27:26
|
Hi Ashutosh, Thanks for your review at first. I compared inherit.out and inherit_1.out under directory regress/expected. There's a lot of differences between these two files. Expected inherit.out keeps the original PG results. Do you think it's necessary to revise the inherit.out impacted by this patch? Thanks & Regards, Benny Wang 2013/2/27 Ashutosh Bapat <ash...@en...> > Hi Benny, > I took a good look at this patch now. Attached please find a revised > patch, with some minor modifications done. Rest of the comments are below > > 1. As a general guideline for adding #ifdef SOMETHING, it's good to end it > with not just #endif but #endif /* SOMETHING */, so that it's easier to > find the mutually corresponding pairs of #ifdef and #endif. Right now I > have added it myself in the attached patch. > > 2. It's better to print the the distribution information at the end of > everything else, so that it's easy to spot in case, someone needs to > differentiate between PG and PGXC output of \d+. This has been taken care > in the revised patch. > > 3. Distribution type and nodes better be on the same line as their heading > e.g. Distributed by: REPLICATION. The patch contains the change. > > 4. As suggested by Nikhil in one of the mails, the ouptut of location > nodes needs to be changed from format {node1,node2,..} to node1, node2, > node3, ... (notice the space after "," and removal of braces.). Done in my > patch. > > Please provide me a patch, with the regression outputs adjusted > accordingly. You will need to change inherit.out along-with inherit_1.out. > > In attached files, print_distribution_info.patch.2 is patch with the > changes described above applied on your patch. > print_distribution_info.patch.diff is the patch containing only the changes > described above. Please review these changes and provide me an updated > patch. > > > On Fri, Feb 22, 2013 at 9:21 AM, Xiong Wang <wan...@gm...>wrote: > >> Hi all, >> >> I finished the patch. If you have any comments, give me a reply. >> >> Thanks & Regards, >> >> Benny Wang >> >> >> 2013/2/21 Koichi Suzuki <koi...@gm...> >> >>> Okay. I hope this satisfies everybody. >>> ---------- >>> Koichi Suzuki >>> >>> >>> 2013/2/20 Ashutosh Bapat <ash...@en...>: >>> > >>> > >>> > On Wed, Feb 20, 2013 at 10:49 AM, Xiong Wang <wan...@gm...> >>> wrote: >>> >> >>> >> Hi Ashutosh, >>> >> >>> >> 2013/2/6 Ashutosh Bapat <ash...@en...> >>> >>> >>> >>> Hi Xiong, >>> >>> >>> >>> On Tue, Feb 5, 2013 at 8:06 PM, Xiong Wang <wan...@gm...> >>> >>> wrote: >>> >>>> >>> >>>> Hi Ashutosh, >>> >>>> 2013/2/5 Ashutosh Bapat <ash...@en...> >>> >>>>> >>> >>>>> Hi Xiong, >>> >>>>> Thanks for the patch. It's very much awaited feature. >>> >>>>> >>> >>>>> Here are some comments on your patch. >>> >>>>> >>> >>>>> The patch applies well, but has some unwanted white spaces >>> >>>>> [ashutosh@ubuntu coderoot]git apply >>> >>>>> /mnt/hgfs/tmp/print_distribution_info.patch >>> >>>>> /mnt/hgfs/tmp/print_distribution_info.patch:28: space before tab in >>> >>>>> indent. >>> >>>>> "SELECT CASE pclocatortype \n" >>> >>>>> /mnt/hgfs/tmp/print_distribution_info.patch:35: trailing >>> whitespace. >>> >>>>> "WHEN '%c' THEN 'MODULO' END || ' ('|| >>> a.attname >>> >>>>> ||')' as distype\n" >>> >>>>> /mnt/hgfs/tmp/print_distribution_info.patch:59: trailing >>> whitespace. >>> >>>>> >>> >>>>> warning: 3 lines add whitespace errors. >>> >>>>> Please take care of those. >>> >>>> >>> >>>> >>> >>>> Thanks for your patient review. I will fix these problems. >>> >>>> >>> >>>>> >>> >>>>> >>> >>>>> I see that there are comments like /* NOTICE: The number of >>> beginning >>> >>>>> whitespace is the same as index print */ followed by printing of a >>> message >>> >>>>> with some spaces hard-coded in it. I do not see this style being >>> used >>> >>>>> anywhere in the file and it looks problematic. If it happens that >>> this new >>> >>>>> information is indented, the hard-coded spaces will not align >>> properly. Can >>> >>>>> you please check what's the proper way of aligning the lines and >>> use that >>> >>>>> method? >>> >>>> >>> >>>> I add this notice deliberately because the length of white spaces >>> before >>> >>>> printing index information is 4. There is no warn similar with my >>> comment in >>> >>>> describe.c. So, I will delete this comment within later patch. >>> Thanks again. >>> >>>> >>> >>> >>> >>> >>> >>> Don't just delete the comment, we need to get rid of hardcoded white >>> >>> spaces. Do you see any other instance in the file which uses white >>> spaces? >>> >> >>> >> >>> >> Yes. There are several other places use hardcoded white spaces such >>> as >>> >> printing constraints including check, fk and printing trigger >>> informations. >>> >> In order to follow postgresql style, I will just delete my comments. >>> >>> >>> >>> >>> >>>>> >>> >>>>> Instead of following query, >>> >>>>> 1742 "SELECT node_name FROM >>> >>>>> pg_catalog.pgxc_node \n" >>> >>>>> 1743 "WHERE oid::text in \n" >>> >>>>> 1744 "(SELECT >>> >>>>> pg_catalog.regexp_split_to_table(nodeoids::text, E'\\\\s+') FROM >>> >>>>> pg_catalog.pgxc_class WHERE pcrelid = '%s');" >>> >>>>> >>> >>>>> I would use (with proper indentation) >>> >>>>> SELECT ARRAY(SELECT node_name FROM pg_catalog.pgxc_node WHERE oid >>> IN >>> >>>>> (SELECT unnest(nodeoids) FROM pgxc_class WHERE pcrelid = %s)); >>> >>>>> This query will give you only one row containing all the nodes. >>> Using >>> >>>>> unnest to convert an array to table and then using IN operator is >>> better >>> >>>>> than converting array to string and using split on string, and then >>> >>>>> combining the result back. That way, we don't rely on the syntax >>> of array to >>> >>>>> string conversion or any particular regular expression. >>> >>>> >>> >>>> Great. I didn't find the unnest function. I will change my query >>> later. >>> >>>> >>> >>>>> >>> >>>>> Please provide the fix for the failing regressions as well. You >>> will >>> >>>>> need to change the expected output. >>> >>>> >>> >>>> As for regression failure, I wanted to submit the fixing patch but >>> my >>> >>>> test environment is different from yours. I doubt that my patch for >>> fixing >>> >>>> the failure may be not useful. >>> >>>> >>> >>> >>> >>> >>> >>> Send the expected output changes anyway, we will have to find out a >>> way >>> >>> to fix the regression. >>> >> >>> >> Ok. >>> >> >>> > >>> > Now you have a way to fix the regression as well. Use ALL DATANODES if >>> the >>> > list of nodes contains all the datanodes. We have just seen one >>> objection. >>> > Printing ALL DATANODES looks to have uses other than silencing >>> regressions. >>> > So, it's worth putting it. >>> > >>> >> >>> >> Thanks & Regards >>> >> Benny Wang >>> >>>> >>> >>>> >>> >>>>> >>> >>>>> Rest of the patch looks good. >>> >>>>> >>> >>>>> On Tue, Feb 5, 2013 at 11:41 AM, Xiong Wang < >>> wan...@gm...> >>> >>>>> wrote: >>> >>>>>> >>> >>>>>> Hi all, >>> >>>>>> >>> >>>>>> The enclosure is the patch for showing distribution information. >>> >>>>>> >>> >>>>>> Two sql files, inherit.sql and create_table_like.sql in the >>> regression >>> >>>>>> test will fail. >>> >>>>>> >>> >>>>>> Thanks & Regards, >>> >>>>>> >>> >>>>>> Benny Wang >>> >>>>>> >>> >>>>>> >>> >>>>>> >>> >>>>>> >>> >>>>>> 2013/2/1 Koichi Suzuki <koi...@gm...> >>> >>>>>>> >>> >>>>>>> Yes, it's nice to have. >>> >>>>>>> >>> >>>>>>> I understand there were many discuttions to have it, separate >>> command >>> >>>>>>> or \d and \d+. \d, \d+ extension will not be affected by >>> command >>> >>>>>>> name conflict. I hope we can handle further change both in XC >>> and >>> >>>>>>> PG. I don't see very big difference in comparison of >>> >>>>>>> separate/existing command. Their pros and cons seems to be >>> >>>>>>> comparable. So I think we can decide what is more convenient to >>> >>>>>>> use. >>> >>>>>>> So far, I understand more people prefer \d. It's quite okay >>> with >>> >>>>>>> me. >>> >>>>>>> >>> >>>>>>> In addition, we may want to see each node information (resource, >>> >>>>>>> primary, preferred) and configuration of each nodegroup. Because >>> >>>>>>> this >>> >>>>>>> is quite new to XC, I think it's better to have xc-specific >>> command >>> >>>>>>> such as \xc something. >>> >>>>>>> >>> >>>>>>> Regards; >>> >>>>>>> ---------- >>> >>>>>>> Koichi Suzuki >>> >>>>>>> >>> >>>>>>> >>> >>>>>>> 2013/2/1 Xiong Wang <wan...@gm...>: >>> >>>>>>> > Hi Suzuki, >>> >>>>>>> > According to Ashutosh and ANikhil, It seems that they want to >>> print >>> >>>>>>> > distributed method as well as the location node list using \d+ >>> . >>> >>>>>>> > Are you in favor? >>> >>>>>>> > >>> >>>>>>> > Regards, >>> >>>>>>> > Benny >>> >>>>>>> > >>> >>>>>>> > >>> >>>>>>> > 2013/2/1 Koichi Suzuki <koi...@gm...> >>> >>>>>>> >> >>> >>>>>>> >> One more issue, >>> >>>>>>> >> >>> >>>>>>> >> Does anybody need a command to print node list from pgxc_node >>> and >>> >>>>>>> >> pgxc_group? >>> >>>>>>> >> ---------- >>> >>>>>>> >> Koichi Suzuki >>> >>>>>>> >> >>> >>>>>>> >> >>> >>>>>>> >> 2013/2/1 Koichi Suzuki <koi...@gm...>: >>> >>>>>>> >> > Great! >>> >>>>>>> >> > >>> >>>>>>> >> > Benny, please post your patch when ready. >>> >>>>>>> >> > ---------- >>> >>>>>>> >> > Koichi Suzuki >>> >>>>>>> >> > >>> >>>>>>> >> > >>> >>>>>>> >> > 2013/2/1 Mason Sharp <ma...@st...>: >>> >>>>>>> >> >> >>> >>>>>>> >> >> >>> >>>>>>> >> >> On Thu, Jan 31, 2013 at 5:38 AM, Ashutosh Bapat >>> >>>>>>> >> >> <ash...@en...> wrote: >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> +1. We should add this functionality as \d+. >>> >>>>>>> >> >> >>> >>>>>>> >> >> >>> >>>>>>> >> >> +1 >>> >>>>>>> >> >> >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> Xion, >>> >>>>>>> >> >>> You will need to output the nodes where the table is >>> >>>>>>> >> >>> distributed or >>> >>>>>>> >> >>> replicated. >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> On Thu, Jan 31, 2013 at 3:11 PM, Nikhil Sontakke >>> >>>>>>> >> >>> <ni...@st...> >>> >>>>>>> >> >>> wrote: >>> >>>>>>> >> >>>> >>> >>>>>>> >> >>>> Btw, I vote for showing PGXC output with \d+ and other >>> >>>>>>> >> >>>> extended >>> >>>>>>> >> >>>> commands >>> >>>>>>> >> >>>> only. >>> >>>>>>> >> >>>> >>> >>>>>>> >> >>>> Nikhils >>> >>>>>>> >> >>>> >>> >>>>>>> >> >>>> On Thu, Jan 31, 2013 at 3:08 PM, Nikhil Sontakke >>> >>>>>>> >> >>>> <ni...@st...> >>> >>>>>>> >> >>>> wrote: >>> >>>>>>> >> >>>> > I still do not understand how showing additional stuff >>> in >>> >>>>>>> >> >>>> > the PGXC >>> >>>>>>> >> >>>> > version makes it incompatible with vanilla Postgres? >>> >>>>>>> >> >>>> > >>> >>>>>>> >> >>>> > As you can see, the OP made changes to the *existing* >>> \d >>> >>>>>>> >> >>>> > logic >>> >>>>>>> >> >>>> > which >>> >>>>>>> >> >>>> > is a logical way of doing things. As long as we use >>> #ifdef >>> >>>>>>> >> >>>> > PGXC, I >>> >>>>>>> >> >>>> > do >>> >>>>>>> >> >>>> > not see how printing additional info breaks anything. >>> >>>>>>> >> >>>> > Infact it >>> >>>>>>> >> >>>> > avoids >>> >>>>>>> >> >>>> > users having to learn more stuff. >>> >>>>>>> >> >>>> > >>> >>>>>>> >> >>>> > Regards, >>> >>>>>>> >> >>>> > Nikhils >>> >>>>>>> >> >>>> > >>> >>>>>>> >> >>>> > On Thu, Jan 31, 2013 at 2:59 PM, Michael Paquier >>> >>>>>>> >> >>>> > <mic...@gm...> wrote: >>> >>>>>>> >> >>>> >> On Thu, Jan 31, 2013 at 6:04 PM, Xiong Wang >>> >>>>>>> >> >>>> >> <wan...@gm...> >>> >>>>>>> >> >>>> >> wrote: >>> >>>>>>> >> >>>> >>> >>> >>>>>>> >> >>>> >>> I wrote a simple patch which will show distribution >>> >>>>>>> >> >>>> >>> information >>> >>>>>>> >> >>>> >>> when >>> >>>>>>> >> >>>> >>> you >>> >>>>>>> >> >>>> >>> use \d tablename command. >>> >>>>>>> >> >>>> >> >>> >>>>>>> >> >>>> >> I vote no for that with ¥d. I agree it is useful, but >>> it >>> >>>>>>> >> >>>> >> makes the >>> >>>>>>> >> >>>> >> output >>> >>>>>>> >> >>>> >> inconsistent with vanilla Postgres. And I am sure it >>> >>>>>>> >> >>>> >> creates many >>> >>>>>>> >> >>>> >> failures >>> >>>>>>> >> >>>> >> in regression tests. It has been discussed before to >>> use >>> >>>>>>> >> >>>> >> either a >>> >>>>>>> >> >>>> >> new >>> >>>>>>> >> >>>> >> command with a word or a letter we'll be sure won't >>> be in >>> >>>>>>> >> >>>> >> conflict >>> >>>>>>> >> >>>> >> with >>> >>>>>>> >> >>>> >> vanilla now and at some point in the future. Something >>> >>>>>>> >> >>>> >> like >>> >>>>>>> >> >>>> >> "¥distrib" >>> >>>>>>> >> >>>> >> perhaps? >>> >>>>>>> >> >>>> >> -- >>> >>>>>>> >> >>>> >> Michael Paquier >>> >>>>>>> >> >>>> >> http://michael.otacoo.com >>> >>>>>>> >> >>>> >> >>> >>>>>>> >> >>>> >> >>> >>>>>>> >> >>>> >> >>> >>>>>>> >> >>>> >> >>> ------------------------------------------------------------------------------ >>> >>>>>>> >> >>>> >> Everyone hates slow websites. So do we. >>> >>>>>>> >> >>>> >> Make your web apps faster with AppDynamics >>> >>>>>>> >> >>>> >> Download AppDynamics Lite for free today: >>> >>>>>>> >> >>>> >> http://p.sf.net/sfu/appdyn_d2d_jan >>> >>>>>>> >> >>>> >> _______________________________________________ >>> >>>>>>> >> >>>> >> Postgres-xc-developers mailing list >>> >>>>>>> >> >>>> >> Pos...@li... >>> >>>>>>> >> >>>> >> >>> >>>>>>> >> >>>> >> >>> >>>>>>> >> >>>> >> >>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>> >>>>>>> >> >>>> >> >>> >>>>>>> >> >>>> > >>> >>>>>>> >> >>>> > >>> >>>>>>> >> >>>> > >>> >>>>>>> >> >>>> > -- >>> >>>>>>> >> >>>> > StormDB - http://www.stormdb.com >>> >>>>>>> >> >>>> > The Database Cloud >>> >>>>>>> >> >>>> > Postgres-XC Support and Service >>> >>>>>>> >> >>>> >>> >>>>>>> >> >>>> >>> >>>>>>> >> >>>> >>> >>>>>>> >> >>>> -- >>> >>>>>>> >> >>>> StormDB - http://www.stormdb.com >>> >>>>>>> >> >>>> The Database Cloud >>> >>>>>>> >> >>>> Postgres-XC Support and Service >>> >>>>>>> >> >>>> >>> >>>>>>> >> >>>> >>> >>>>>>> >> >>>> >>> >>>>>>> >> >>>> >>> >>>>>>> >> >>>> >>> ------------------------------------------------------------------------------ >>> >>>>>>> >> >>>> Everyone hates slow websites. So do we. >>> >>>>>>> >> >>>> Make your web apps faster with AppDynamics >>> >>>>>>> >> >>>> Download AppDynamics Lite for free today: >>> >>>>>>> >> >>>> http://p.sf.net/sfu/appdyn_d2d_jan >>> >>>>>>> >> >>>> _______________________________________________ >>> >>>>>>> >> >>>> Postgres-xc-developers mailing list >>> >>>>>>> >> >>>> Pos...@li... >>> >>>>>>> >> >>>> >>> >>>>>>> >> >>>> >>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> -- >>> >>>>>>> >> >>> Best Wishes, >>> >>>>>>> >> >>> Ashutosh Bapat >>> >>>>>>> >> >>> EntepriseDB Corporation >>> >>>>>>> >> >>> The Enterprise Postgres Company >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> >>> ------------------------------------------------------------------------------ >>> >>>>>>> >> >>> Everyone hates slow websites. So do we. >>> >>>>>>> >> >>> Make your web apps faster with AppDynamics >>> >>>>>>> >> >>> Download AppDynamics Lite for free today: >>> >>>>>>> >> >>> http://p.sf.net/sfu/appdyn_d2d_jan >>> >>>>>>> >> >>> _______________________________________________ >>> >>>>>>> >> >>> Postgres-xc-developers mailing list >>> >>>>>>> >> >>> Pos...@li... >>> >>>>>>> >> >>> >>> >>>>>>> >> >>> >>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>> >>>>>>> >> >>> >>> >>>>>>> >> >> >>> >>>>>>> >> >> >>> >>>>>>> >> >> >>> >>>>>>> >> >> -- >>> >>>>>>> >> >> Mason Sharp >>> >>>>>>> >> >> >>> >>>>>>> >> >> >>> >>>>>>> >> >> StormDB - http://www.stormdb.com >>> >>>>>>> >> >> The Database Cloud >>> >>>>>>> >> >> Postgres-XC Support and Services >>> >>>>>>> >> >> >>> >>>>>>> >> >> >>> >>>>>>> >> >> >>> ------------------------------------------------------------------------------ >>> >>>>>>> >> >> Everyone hates slow websites. So do we. >>> >>>>>>> >> >> Make your web apps faster with AppDynamics >>> >>>>>>> >> >> Download AppDynamics Lite for free today: >>> >>>>>>> >> >> http://p.sf.net/sfu/appdyn_d2d_jan >>> >>>>>>> >> >> _______________________________________________ >>> >>>>>>> >> >> Postgres-xc-developers mailing list >>> >>>>>>> >> >> Pos...@li... >>> >>>>>>> >> >> >>> >>>>>>> >> >> >>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>> >>>>>>> >> >> >>> >>>>>>> >> >>> >>>>>>> >> >>> >>>>>>> >> >>> >>>>>>> >> >>> ------------------------------------------------------------------------------ >>> >>>>>>> >> Everyone hates slow websites. So do we. >>> >>>>>>> >> Make your web apps faster with AppDynamics >>> >>>>>>> >> Download AppDynamics Lite for free today: >>> >>>>>>> >> http://p.sf.net/sfu/appdyn_d2d_jan >>> >>>>>>> >> _______________________________________________ >>> >>>>>>> >> Postgres-xc-developers mailing list >>> >>>>>>> >> Pos...@li... >>> >>>>>>> >> >>> >>>>>>> >> >>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>> >>>>>>> > >>> >>>>>>> > >>> >>>>>> >>> >>>>>> >>> >>>>>> >>> >>>>>> >>> >>>>>> >>> ------------------------------------------------------------------------------ >>> >>>>>> Free Next-Gen Firewall Hardware Offer >>> >>>>>> Buy your Sophos next-gen firewall before the end March 2013 >>> >>>>>> and get the hardware for free! Learn more. >>> >>>>>> http://p.sf.net/sfu/sophos-d2d-feb >>> >>>>>> _______________________________________________ >>> >>>>>> Postgres-xc-developers mailing list >>> >>>>>> Pos...@li... >>> >>>>>> >>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>> >>>>>> >>> >>>>> >>> >>>>> >>> >>>>> >>> >>>>> -- >>> >>>>> Best Wishes, >>> >>>>> Ashutosh Bapat >>> >>>>> EntepriseDB Corporation >>> >>>>> The Enterprise Postgres Company >>> >>>> >>> >>>> >>> >>> >>> >>> >>> >>> >>> >>> -- >>> >>> Best Wishes, >>> >>> Ashutosh Bapat >>> >>> EntepriseDB Corporation >>> >>> The Enterprise Postgres Company >>> >> >>> >> >>> > >>> > >>> > >>> > -- >>> > Best Wishes, >>> > Ashutosh Bapat >>> > EntepriseDB Corporation >>> > The Enterprise Postgres Company >>> > >>> > >>> ------------------------------------------------------------------------------ >>> > Everyone hates slow websites. So do we. >>> > Make your web apps faster with AppDynamics >>> > Download AppDynamics Lite for free today: >>> > http://p.sf.net/sfu/appdyn_d2d_feb >>> > _______________________________________________ >>> > Postgres-xc-developers mailing list >>> > Pos...@li... >>> > https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>> > >>> >> >> >> >> ------------------------------------------------------------------------------ >> Everyone hates slow websites. So do we. >> Make your web apps faster with AppDynamics >> Download AppDynamics Lite for free today: >> http://p.sf.net/sfu/appdyn_d2d_feb >> _______________________________________________ >> Postgres-xc-developers mailing list >> Pos...@li... >> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >> >> > > > -- > Best Wishes, > Ashutosh Bapat > EntepriseDB Corporation > The Enterprise Postgres Company > |