Is there any point using pg_get_triggerdef in pg_dump to generate trigger
definitions?  We'd still have to keep the old code so that we can dump pre
7.4, but it might mean we don't have to touch that code again if we add
triggers on columns or something...

Also, it doesn't format them as nicely as the current pg_dump code...

Chris


Tue, 17 Jun 2003 10:01:26 +0800
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au>
pg_get_triggerdef in pg_dump
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
> Is there any point using pg_get_triggerdef in pg_dump to generate trigger
> definitions?  We'd still have to keep the old code so that we can dump pre
> 7.4, but it might mean we don't have to touch that code again if we add
> triggers on columns or something...

Seems like a good idea to me --- we've been trying to reduce pg_dump's
knowledge of backend nitty-gritty, and this would be another small step
in the right direction.

> Also, it doesn't format them as nicely as the current pg_dump code...

That's fixable no?  I guess you might want to consider what psql's \d
display will look like too, but I don't recall that we ever promised
anyone that the pg_get_xxx functions would output no unnecessary
whitespace.

			regards, tom lane

Mon, 16 Jun 2003 23:44:56 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pg_get_triggerdef in pg_dump 
> Seems like a good idea to me --- we've been trying to reduce pg_dump's
> knowledge of backend nitty-gritty, and this would be another small step
> in the right direction.
>
> > Also, it doesn't format them as nicely as the current pg_dump code...
>
> That's fixable no?  I guess you might want to consider what psql's \d
> display will look like too, but I don't recall that we ever promised
> anyone that the pg_get_xxx functions would output no unnecessary
> whitespace.

We make pg_get_xxx2 functions that return a formatted version.  Internally,
we just add an extra boolean parameter to the pg_get_triggerdef() function
in ruleutils and we call that true or false depending...

Chris


Tue, 17 Jun 2003 11:53:34 +0800
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au>
Re: pg_get_triggerdef in pg_dump 
Christopher Kings-Lynne wrote:

>
>We make pg_get_xxx2 functions that return a formatted version.  Internally,
>we just add an extra boolean parameter to the pg_get_triggerdef() function
>in ruleutils and we call that true or false depending...
>
>  
>
That's what I got too!
Several weeks ago I proposed such functions as contribute module to this 
list, with no result. Seems I'm not the only one that wants to read 
his/her trigger/view/rules after pushing them into pgsql...
Difference from Christopher's solution is that mine utilizes completely 
separatated (copied) code, so ruleutils code is still unchanged. This 
was a concession to Tom who claimed concerns about pg_dump not being 
able to reproduce things correctly if there was *any* error in it.
Maybe we get some progress now on this topic?

Regards,
Andreas





Tue, 17 Jun 2003 10:16:02 +0200
Andreas Pflug <Andreas.Pflug@web.de>
Re: pg_get_triggerdef in pg_dump
Andreas Pflug <Andreas.Pflug@web.de> writes:
> Difference from Christopher's solution is that mine utilizes completely 
> separatated (copied) code, so ruleutils code is still unchanged. This 
> was a concession to Tom who claimed concerns about pg_dump not being 
> able to reproduce things correctly if there was *any* error in it.

I recall objecting to someone who wanted to remove "unnecessary"
parentheses, but I can't see any risk in inserting unnecessary
whitespace.

			regards, tom lane

Tue, 17 Jun 2003 10:09:18 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pg_get_triggerdef in pg_dump 
Tom Lane wrote:

>
>I recall objecting to someone who wanted to remove "unnecessary"
>parentheses, but I can't see any risk in inserting unnecessary
>whitespace.
>
That "someone" was me indeed, and as I mentioned the code is completely 
separated from the code that pg_dump uses. Thus, there's *no way* that 
this could break backup integrity. I consider these original functions 
as pg_dump helper functions, not meant to be human readable.

There are *many* parentheses that are not necessary, and the code trying 
to figure out is quite conservative. All is decided in one single 
routine, depending on two parameters only, and thus failing to locate 
several cases when parentheses would be avoidable (not even */ over +- 
will be noticed!).

I've been trying hard to make pgsql as maintainable as mssql, and 
there's only this point left. Any attempts to contribute this so far 
just have been answered with "dunno but there might eventually perhaps 
maybe some problem" without having a look at that function. I feel that 
I am asked to prove the validity of my code, which is as impossible as 
it is for software in general, but I haven't seen any case where my 
solution failed to reproduce correctly. If you know one, tell me. If you 
know a case where my core routine decides falsely, tell me.

What I *really* want is having the original source stored, including 
comments, version info, ... Currently, it's argued that underlying table 
and column might change, braking the view/rule. This could be 
restricted, or source could be dropped (alter table ... cascaded). Is it 
really only me who  tries to put complicated views into pgsql and wants 
to understand them 10 days later? We do have an enterprise grade RDBMS, 
don't we?

Regards,
Andreas


Tue, 17 Jun 2003 16:44:39 +0200
Andreas Pflug <Andreas.Pflug@web.de>
Re: pg_get_triggerdef in pg_dump
> What I *really* want is having the original source stored, including 
> comments, version info, ... Currently, it's argued that underlying table 
> and column might change, braking the view/rule. This could be 
> restricted, or source could be dropped (alter table ... cascaded). Is it 
> really only me who  tries to put complicated views into pgsql and wants 
> to understand them 10 days later? We do have an enterprise grade RDBMS, 
> don't we?

You could argue that comments should be converted to an 'information'
node within the query structure which contains comments.  They would
then be dumped back out to the user.

But I think you would be dissapointed if you were returned the view that
is no longer correct since someone renamed the tables.

-- 
Rod Taylor <rbt@rbt.ca>

PGP Key: http://www.rbt.ca/rbtpub.asc
17 Jun 2003 19:11:43 +0000
Rod Taylor <rbt@rbt.ca>
Re: pg_get_triggerdef in pg_dump
Rod Taylor wrote:

>>What I *really* want is having the original source stored, including 
>>comments, version info, ... Currently, it's argued that underlying table 
>>and column might change, braking the view/rule. This could be 
>>restricted, or source could be dropped (alter table ... cascaded). Is it 
>>really only me who  tries to put complicated views into pgsql and wants 
>>to understand them 10 days later? We do have an enterprise grade RDBMS, 
>>don't we?
>>    
>>
>
>You could argue that comments should be converted to an 'information'
>node within the query structure which contains comments.  They would
>then be dumped back out to the user.
>
>But I think you would be dissapointed if you were returned the view that
>is no longer correct since someone renamed the tables.
>
>  
>
Rod,
this arguments are quite academic. On one side, this could be 
restricted, thats what pg_depends is good for (this already happens for 
inherited tables).
On the other side, how often do you rename columns or tables?
On mssql, nobody cares. If you fool around with names, your views will 
be broken without warning. pgsql could be better easily.
I'd really prefer to have full view sources available rather than the 
gimmick of stable views despite renamed cols/tabs.

Regards,
Andreas


Tue, 17 Jun 2003 23:45:34 +0200
Andreas Pflug <Andreas.Pflug@web.de>
Re: pg_get_triggerdef in pg_dump
> this arguments are quite academic. 

You what!

> On one side, this could be 
> restricted, thats what pg_depends is good for (this already happens for 
> inherited tables).
> On the other side, how often do you rename columns or tables?

You what!

> On mssql, nobody cares. 

You what!

> If you fool around with names, your views will 
> be broken without warning. pgsql could be better easily.
> I'd really prefer to have full view sources available rather than the 
> gimmick of stable views despite renamed cols/tabs.

Gimmick!  You what!!!!!!



Wed, 18 Jun 2003 09:39:54 +0800
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au>
Re: pg_get_triggerdef in pg_dump
Christopher Kings-Lynne wrote:

>>this arguments are quite academic. 
>>    
>>
>
>You what!
>
>  
>
>>On one side, this could be 
>>restricted, thats what pg_depends is good for (this already happens for 
>>inherited tables).
>>On the other side, how often do you rename columns or tables?
>>    
>>
>
>You what!
>
>  
>
>>On mssql, nobody cares. 
>>    
>>
>
>You what!
>
>  
>
>>If you fool around with names, your views will 
>>be broken without warning. pgsql could be better easily.
>>I'd really prefer to have full view sources available rather than the 
>>gimmick of stable views despite renamed cols/tabs.
>>    
>>
>
>Gimmick!  You what!!!!!!
>
>  
>
Christopher,

I'm not natively english speaking, and so I don't understand what you 
want to say with this. Maybe this is some kind of Australian slang? Do 
you agree or disagree? I'm trying to explain my concerns and proposals, 
and it would be kind if I'm answered seriously and understandably.

Regards,
Andreas


Wed, 18 Jun 2003 11:55:46 +0200
Andreas Pflug <Andreas.Pflug@web.de>
Re: pg_get_triggerdef in pg_dump
Hi Andreas,

> I'm not natively english speaking, and so I don't understand what you
> want to say with this. Maybe this is some kind of Australian slang? Do
> you agree or disagree? I'm trying to explain my concerns and proposals,
> and it would be kind if I'm answered seriously and understandably.

Sorry if I offended you.  'You what!' is what you say when you cannot
believe what someone is saying...  Calling 'stable views that work when
you rename columns' a gimmick is quite an incredible thing to say...

You honestly would rather be able to view accurate source of views that
don't work rather than complicted source of views that actually work?

Anyway, there's no reason why we can't have both with a bit of effort...

Chris



Wed, 18 Jun 2003 18:15:08 +0800 (WST)
Christopher Kings-Lynne <chriskl@familyhealth.com.au>
Re: pg_get_triggerdef in pg_dump
Hi Christopher,

>Sorry if I offended you.
>
I wasn't offended because I wasn't sure what you wanted to say to me. 
You may call me the biggest fool of all, as long you do it in Sualheli, 
or Korean... :-)

>  'You what!' is what you say when you cannot
>believe what someone is saying...  Calling 'stable views that work when
>you rename columns' a gimmick is quite an incredible thing to say...
>
>You honestly would rather be able to view accurate source of views that
>don't work rather than complicted source of views that actually work?
>
Yes, that's right. I've been working this way for years, and all MSSQL 
users do because there's no other way for them.
These automatic name change propagation is very limited, concerning all 
possible changes you can have in a table.

- drop column -> will restrict now, or need cascade
- rename column -> propagates to plan tree, why not restrict or require 
cascaded to drop source?
- alter column size/type -> not possible, need to create alternate 
column, drop old (which is restricted...) and rename

You see, only few changes that can be done are handled at the moment.

In my experience with large and complicated data models, I found that I 
hardly ever would rename a table or a column. There's nothing like an 
automatic column name update of applications... After years, it's hard 
to tell where everything's used, especially if queries are created at 
runtime.
I consider a view more as being a part of an application, rather than 
part of the data model (unless rules are used), and thus the same 
problems apply.

What I need again and again, is changing the size of a column (rarely 
the type). For pgsql, I'd have to drop the column, and need to recreate 
all views. For MSSQL, it won't matter if the column is dropped/recreated 
or just resized, the view won't notice until it's used again.

>Anyway, there's no reason why we can't have both with a bit of effort...
>  
>
This certainly would be nice, maybe there could be back-pointers from 
nodes into the source so identifier names can be identified and 
modified? Just like debugging-enabled code has references to the source.

Another way could be storing the source in a translated form, like
SELECT X.($88012.1) FROM $88012 AS X
instead of
SELECT X.bar FROM public.foo AS X


Regards,
Andreas


Wed, 18 Jun 2003 12:59:36 +0200
Andreas Pflug <Andreas.Pflug@web.de>
Re: pg_get_triggerdef in pg_dump
On Wed, Jun 18, 2003 at 12:59:36PM +0200, Andreas Pflug wrote:

> What I need again and again, is changing the size of a column (rarely 
> the type). For pgsql, I'd have to drop the column, and need to recreate 
> all views. For MSSQL, it won't matter if the column is dropped/recreated 
> or just resized, the view won't notice until it's used again.

If that's what you need you can always change the system catalogs
manually.  For CHAR(n) and VARCHAR(n) you change pg_attribute.atttypmod
to (n+4).  For NUMERIC(n,m) it's something like (n<<16) + m + 4 or maybe
(m<<16) + n + 4, don't remember right now.

Be sure to check that your data is in a safe place before you do this,
and double check before you commit the transaction if you do it
manually.

-- 
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Porque Kim no hacia nada, pero, eso si,
con extraordinario exito" ("Kim", Kipling)

Wed, 18 Jun 2003 10:35:37 -0400
Alvaro Herrera <alvherre@dcc.uchile.cl>
Re: pg_get_triggerdef in pg_dump
Alvaro Herrera wrote:

>On Wed, Jun 18, 2003 at 12:59:36PM +0200, Andreas Pflug wrote:
>
>  
>
>>What I need again and again, is changing the size of a column (rarely 
>>the type). For pgsql, I'd have to drop the column, and need to recreate 
>>all views. For MSSQL, it won't matter if the column is dropped/recreated 
>>or just resized, the view won't notice until it's used again.
>>    
>>
>
>If that's what you need you can always change the system catalogs
>manually.  For CHAR(n) and VARCHAR(n) you change pg_attribute.atttypmod
>to (n+4).  For NUMERIC(n,m) it's something like (n<<16) + m + 4 or maybe
>(m<<16) + n + 4, don't remember right now.
>
>Be sure to check that your data is in a safe place before you do this,
>and double check before you commit the transaction if you do it
>manually.
>
>  
>
Hm, you're right, 'thou I wouldn't recommend this to the average user, 
and wonder if this will be possible for all future pgsql versions too. 
I'm considering adding safe support for this type of column change to 
pgAdmin3.
There might be other cases of legal direct change of system catalog 
entries, e,g. varchar to text, if they all are only names for internally 
identical data structures. Can you tell which datatypes may be legally 
interchanged?

Regards,
Andreas




Wed, 18 Jun 2003 17:57:41 +0200
Andreas Pflug <Andreas.Pflug@web.de>
Re: pg_get_triggerdef in pg_dump
> There might be other cases of legal direct change of system catalog 
> entries, e,g. varchar to text, if they all are only names for internally 
> identical data structures. Can you tell which datatypes may be legally 
> interchanged?

If pg_cast.castfunc is 0, you should might be able to do a datatype
change safely.

http://candle.pha.pa.us/main/writings/pgsql/sgml/catalog-pg-cast.html#AEN49071

-- 
Rod Taylor <rbt@rbt.ca>

PGP Key: http://www.rbt.ca/rbtpub.asc
18 Jun 2003 16:16:51 +0000
Rod Taylor <rbt@rbt.ca>
Re: pg_get_triggerdef in pg_dump
Andreas Pflug <Andreas.Pflug@web.de> writes:
> There might be other cases of legal direct change of system catalog 
> entries, e,g. varchar to text, if they all are only names for internally 
> identical data structures. Can you tell which datatypes may be legally 
> interchanged?

Right offhand I think text<->varchar and adjustment of length limits in
char, varchar, and perhaps numeric would be the only things useful
enough to worry about handling.

			regards, tom lane

Wed, 18 Jun 2003 12:24:59 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pg_get_triggerdef in pg_dump 
> If that's what you need you can always change the system catalogs
> manually.  For CHAR(n) and VARCHAR(n) you change pg_attribute.atttypmod
> to (n+4).  For NUMERIC(n,m) it's something like (n<<16) + m + 4 or maybe
> (m<<16) + n + 4, don't remember right now.
>
> Be sure to check that your data is in a safe place before you do this,
> and double check before you commit the transaction if you do it
> manually.

Is there demand for an ALTER COLUMN/SET TYPE that is restricted to binary
compatible casts and increasing length changes?

Chris


Thu, 19 Jun 2003 09:18:50 +0800
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au>
Re: pg_get_triggerdef in pg_dump
> Hm, you're right, 'thou I wouldn't recommend this to the average user, 
> and wonder if this will be possible for all future pgsql versions too. 
> I'm considering adding safe support for this type of column change to 
> pgAdmin3.
> There might be other cases of legal direct change of system catalog 
> entries, e,g. varchar to text, if they all are only names for internally 
> identical data structures. Can you tell which datatypes may be legally 
> interchanged?

Yes, you can check if they're binary compatible from the pg_cast table....

Chris


Thu, 19 Jun 2003 09:22:03 +0800
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au>
Re: pg_get_triggerdef in pg_dump
> Right offhand I think text<->varchar and adjustment of length limits in
> char, varchar, and perhaps numeric would be the only things useful
> enough to worry about handling.

I'd love to have adding and removing precision and timezones on timestamp*
fields

Chris


Thu, 19 Jun 2003 09:23:49 +0800
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au>
Re: pg_get_triggerdef in pg_dump 
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
>> There might be other cases of legal direct change of system catalog 
>> entries, e,g. varchar to text, if they all are only names for internally 
>> identical data structures. Can you tell which datatypes may be legally 
>> interchanged?

> Yes, you can check if they're binary compatible from the pg_cast table....

But nearly all of the interesting cases require you to understand the
type's interpretation of typmod, and you can't learn that from a table.
How many cases are there where blindly looking for a binary-compatible
cast in pg_cast will really do you much good?  AFAICS you'd have to set
atttypmod to -1 if you change atttypid without knowing very specifically
what you are changing from and to.

			regards, tom lane

Wed, 18 Jun 2003 22:33:17 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pg_get_triggerdef in pg_dump 
Tom Lane wrote:

>>Yes, you can check if they're binary compatible from the pg_cast table....
>>    
>>
>
>But nearly all of the interesting cases require you to understand the
>type's interpretation of typmod, and you can't learn that from a table.
>How many cases are there where blindly looking for a binary-compatible
>cast in pg_cast will really do you much good?  AFAICS you'd have to set
>atttypmod to -1 if you change atttypid without knowing very specifically
>what you are changing from and to.
>
>  
>
AFAICS there's few interpretation about atttypmod necessary because only 
few datatypes binary convertible (castfunc=0) do use atttypmod at all. 
Most special case is varchar<->text, one supporting length, the other 
not; both need atttypmod=-1. bpchar<->varchar both allow typmod in a 
similar fashion.
It's already implemented in pgAdmin3 this way.

Regards,
Andreas


Thu, 19 Jun 2003 12:17:20 +0200
Andreas Pflug <Andreas.Pflug@web.de>
Re: pg_get_triggerdef in pg_dump

OK, added to TODO:

	Modify pg_get_triggerdef() to take a boolean to pretty-print,
	and use that as part of pg_dump along with psql

Andreas, can you work on this?  I like the idea of removing extra
parens, and merging it into the existing code rather than into contrib
makes sense.

---------------------------------------------------------------------------

Andreas Pflug wrote:
> Tom Lane wrote:
> 
> >
> >I recall objecting to someone who wanted to remove "unnecessary"
> >parentheses, but I can't see any risk in inserting unnecessary
> >whitespace.
> >
> That "someone" was me indeed, and as I mentioned the code is completely 
> separated from the code that pg_dump uses. Thus, there's *no way* that 
> this could break backup integrity. I consider these original functions 
> as pg_dump helper functions, not meant to be human readable.
> 
> There are *many* parentheses that are not necessary, and the code trying 
> to figure out is quite conservative. All is decided in one single 
> routine, depending on two parameters only, and thus failing to locate 
> several cases when parentheses would be avoidable (not even */ over +- 
> will be noticed!).
> 
> I've been trying hard to make pgsql as maintainable as mssql, and 
> there's only this point left. Any attempts to contribute this so far 
> just have been answered with "dunno but there might eventually perhaps 
> maybe some problem" without having a look at that function. I feel that 
> I am asked to prove the validity of my code, which is as impossible as 
> it is for software in general, but I haven't seen any case where my 
> solution failed to reproduce correctly. If you know one, tell me. If you 
> know a case where my core routine decides falsely, tell me.
> 
> What I *really* want is having the original source stored, including 
> comments, version info, ... Currently, it's argued that underlying table 
> and column might change, braking the view/rule. This could be 
> restricted, or source could be dropped (alter table ... cascaded). Is it 
> really only me who  tries to put complicated views into pgsql and wants 
> to understand them 10 days later? We do have an enterprise grade RDBMS, 
> don't we?
> 
> Regards,
> Andreas
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index scan if your
>       joining column's datatypes do not match
> 

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Mon, 23 Jun 2003 22:41:05 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: pg_get_triggerdef in pg_dump

Add to TODO:

        o Allow ALTER TABLE to modify column lengths and change to binary
          compatible types

---------------------------------------------------------------------------

Tom Lane wrote:
> "Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
> >> There might be other cases of legal direct change of system catalog 
> >> entries, e,g. varchar to text, if they all are only names for internally 
> >> identical data structures. Can you tell which datatypes may be legally 
> >> interchanged?
> 
> > Yes, you can check if they're binary compatible from the pg_cast table....
> 
> But nearly all of the interesting cases require you to understand the
> type's interpretation of typmod, and you can't learn that from a table.
> How many cases are there where blindly looking for a binary-compatible
> cast in pg_cast will really do you much good?  AFAICS you'd have to set
> atttypmod to -1 if you change atttypid without knowing very specifically
> what you are changing from and to.
> 
> 			regards, tom lane
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
> 

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Mon, 23 Jun 2003 22:52:02 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: pg_get_triggerdef in pg_dump


Bruce Momjian wrote:

>OK, added to TODO:
>
>	Modify pg_get_triggerdef() to take a boolean to pretty-print,
>	and use that as part of pg_dump along with psql
>
>Andreas, can you work on this?  I like the idea of removing extra
>parens, and merging it into the existing code rather than into contrib
>makes sense.
>

Yes, I can. At the moment, I have a runnable contrib module, which 
replaces all pg_get_xxxdef by pg_get_xxxdef2 functions. It's no problem 
to apply that code back to the original ruleutils.c when the 
isSimpleNode algorithm is reviewed independently and proved being correct.

For safety reasons, I can make this dependent on a bool pretty-print 
parameter.

I also could implement line break and indentation formatting. I 
implemented a keyword-based algorithm in pgAdmin3, and having the 
original tree the job is obviously easier. Do we need any flexibility 
about indent char (tab or space) and indentation size (2 chars)? The 
pgAdmin3 algorithm uses 4 spaces, and tries to align keywords so they 
line up nicely, and I'd prefer doing it this way again.

 SELECT foo
   FROM bar b
   JOIN chair c USING (thekeycol)
  WHERE ...
    


Regards,

Andreas

/***************************************
 * check if given node is simple.
 *  false  : not simple
 *  true   : simple in the context of parent node's type
 ***************************************/

static bool isSimpleNode(Node *node, NodeTag parentNodeType)
{
    if (!node)
	return true;

    switch (nodeTag(node))
    {
	// single words: always simple
	case T_Var:
	case T_Const:
	case T_Param:

	// function-like: name(..) or name[..]
	case T_ArrayRef:
	case T_FuncExpr:
	case T_ArrayExpr:
	case T_CoalesceExpr:
	case T_NullIfExpr:
	case T_Aggref:

        // CASE keywords act as parentheses
	case T_CaseExpr:
	    return true;

        // appears simple since . has top precedence, unless parent is T_FieldSelect itself!
	case T_FieldSelect:
	    return (parentNodeType == T_FieldSelect ? false : true);


        // maybe simple, check args
	case T_CoerceToDomain:
	    return isSimpleNode((Node*) ((CoerceToDomain*)node)->arg, T_CoerceToDomain);
	case T_RelabelType:
	    return isSimpleNode((Node*) ((RelabelType*)node)->arg, T_RelabelType);


	// depends on parent node type; needs further checking
	case T_SubLink:
	case T_NullTest:
	case T_BooleanTest:
	case T_OpExpr:
	case T_DistinctExpr:
	    if (parentNodeType == T_BoolExpr)
		return true;
        // else check the same as for T_BoolExpr; no break here!
	case T_BoolExpr:
	    switch (parentNodeType)
	    {
		case T_ArrayRef:
		case T_FuncExpr:
		case T_ArrayExpr:
		case T_CoalesceExpr:
		case T_NullIfExpr:
		case T_Aggref:
		case T_CaseExpr:
		    return true;
		default:
		    break;
	    }
	    return false;

        // these are not completely implemented; so far, they're simple
	case T_SubPlan:
	case T_CoerceToDomainValue:
	    return true;

	default:
	    break;
    }
    // those we don't know: in dubio complexo
    return false;
}





Tue, 24 Jun 2003 12:38:40 +0200
Andreas Pflug <Andreas.Pflug@web.de>
Re: pg_get_triggerdef in pg_dump

Great.  I recommend using spaces rather than tabs for indenting in psql
and pg_dump.

---------------------------------------------------------------------------

Andreas Pflug wrote:
> 
> 
> Bruce Momjian wrote:
> 
> >OK, added to TODO:
> >
> >	Modify pg_get_triggerdef() to take a boolean to pretty-print,
> >	and use that as part of pg_dump along with psql
> >
> >Andreas, can you work on this?  I like the idea of removing extra
> >parens, and merging it into the existing code rather than into contrib
> >makes sense.
> >
> 
> Yes, I can. At the moment, I have a runnable contrib module, which 
> replaces all pg_get_xxxdef by pg_get_xxxdef2 functions. It's no problem 
> to apply that code back to the original ruleutils.c when the 
> isSimpleNode algorithm is reviewed independently and proved being correct.
> 
> For safety reasons, I can make this dependent on a bool pretty-print 
> parameter.
> 
> I also could implement line break and indentation formatting. I 
> implemented a keyword-based algorithm in pgAdmin3, and having the 
> original tree the job is obviously easier. Do we need any flexibility 
> about indent char (tab or space) and indentation size (2 chars)? The 
> pgAdmin3 algorithm uses 4 spaces, and tries to align keywords so they 
> line up nicely, and I'd prefer doing it this way again.
> 
>  SELECT foo
>    FROM bar b
>    JOIN chair c USING (thekeycol)
>   WHERE ...
>     
> 
> 
> Regards,
> 
> Andreas
> 
> /***************************************
>  * check if given node is simple.
>  *  false  : not simple
>  *  true   : simple in the context of parent node's type
>  ***************************************/
> 
> static bool isSimpleNode(Node *node, NodeTag parentNodeType)
> {
>     if (!node)
> 	return true;
> 
>     switch (nodeTag(node))
>     {
> 	// single words: always simple
> 	case T_Var:
> 	case T_Const:
> 	case T_Param:
> 
> 	// function-like: name(..) or name[..]
> 	case T_ArrayRef:
> 	case T_FuncExpr:
> 	case T_ArrayExpr:
> 	case T_CoalesceExpr:
> 	case T_NullIfExpr:
> 	case T_Aggref:
> 
>         // CASE keywords act as parentheses
> 	case T_CaseExpr:
> 	    return true;
> 
>         // appears simple since . has top precedence, unless parent is T_FieldSelect itself!
> 	case T_FieldSelect:
> 	    return (parentNodeType == T_FieldSelect ? false : true);
> 
> 
>         // maybe simple, check args
> 	case T_CoerceToDomain:
> 	    return isSimpleNode((Node*) ((CoerceToDomain*)node)->arg, T_CoerceToDomain);
> 	case T_RelabelType:
> 	    return isSimpleNode((Node*) ((RelabelType*)node)->arg, T_RelabelType);
> 
> 
> 	// depends on parent node type; needs further checking
> 	case T_SubLink:
> 	case T_NullTest:
> 	case T_BooleanTest:
> 	case T_OpExpr:
> 	case T_DistinctExpr:
> 	    if (parentNodeType == T_BoolExpr)
> 		return true;
>         // else check the same as for T_BoolExpr; no break here!
> 	case T_BoolExpr:
> 	    switch (parentNodeType)
> 	    {
> 		case T_ArrayRef:
> 		case T_FuncExpr:
> 		case T_ArrayExpr:
> 		case T_CoalesceExpr:
> 		case T_NullIfExpr:
> 		case T_Aggref:
> 		case T_CaseExpr:
> 		    return true;
> 		default:
> 		    break;
> 	    }
> 	    return false;
> 
>         // these are not completely implemented; so far, they're simple
> 	case T_SubPlan:
> 	case T_CoerceToDomainValue:
> 	    return true;
> 
> 	default:
> 	    break;
>     }
>     // those we don't know: in dubio complexo
>     return false;
> }
> 
> 
> 
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
> 
>                http://www.postgresql.org/docs/faqs/FAQ.html
> 

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Tue, 24 Jun 2003 09:34:58 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: pg_get_triggerdef in pg_dump
Andreas Pflug wrote:
> I also could implement line break and indentation formatting. I 
> implemented a keyword-based algorithm in pgAdmin3, and having the 
> original tree the job is obviously easier. Do we need any flexibility 
> about indent char (tab or space) and indentation size (2 chars)? The 
> pgAdmin3 algorithm uses 4 spaces, and tries to align keywords so they 
> line up nicely, and I'd prefer doing it this way again.
> 
>  SELECT foo
>    FROM bar b
>    JOIN chair c USING (thekeycol)
>   WHERE ...

Oh, one more thing --- right justify isn't as accepted as left-justify
--- sorry.

>  SELECT foo
>  FROM   bar b
>  JOIN   chair c USING (thekeycol)
>  WHERE ...

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Tue, 24 Jun 2003 09:36:01 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: pg_get_triggerdef in pg_dump
> Oh, one more thing --- right justify isn't as accepted as left-justify

But it looks so much better...

-- 
Rod Taylor <rbt@rbt.ca>

PGP Key: http://www.rbt.ca/rbtpub.asc
24 Jun 2003 11:02:56 -0400
Rod Taylor <rbt@rbt.ca>
Re: pg_get_triggerdef in pg_dump
Rod Taylor wrote:

>>Oh, one more thing --- right justify isn't as accepted as left-justify
>>    
>>
>
>But it looks so much better...
>  
>
Yessss!
Consider this:

SELECT foo
  FROM bar b
  LEFT JOIN chair c USING (thekeycol)
 WHERE ...
:-)

versus

SELECT foo
FROM   bar b
LEFT   JOIN chair c USING (thekeycol)
WHERE  ...
The keywords are separated :-(

SELECT foo
FROM   bar b
LEFT JOIN chair c USING (thekeycol)
WHERE  ...
No more lineup :-(



Admittedly, when you type it yourself, it's a bit annoying, because you 
can't use just tabs. But if it's generated, it won't do any harm.
Why not giving PostgreSQL this extra portion of elegance...

Regards,
Andreas


Tue, 24 Jun 2003 20:57:59 +0200
Andreas Pflug <Andreas.Pflug@web.de>
Re: pg_get_triggerdef in pg_dump

I don't think an option to do such justification would be good unless we
can do it consistenly in the code, and there is enough demand.

---------------------------------------------------------------------------

Andreas Pflug wrote:
> Rod Taylor wrote:
> 
> >>Oh, one more thing --- right justify isn't as accepted as left-justify
> >>    
> >>
> >
> >But it looks so much better...
> >  
> >
> Yessss!
> Consider this:
> 
> SELECT foo
>   FROM bar b
>   LEFT JOIN chair c USING (thekeycol)
>  WHERE ...
> :-)
> 
> versus
> 
> SELECT foo
> FROM   bar b
> LEFT   JOIN chair c USING (thekeycol)
> WHERE  ...
> The keywords are separated :-(
> 
> SELECT foo
> FROM   bar b
> LEFT JOIN chair c USING (thekeycol)
> WHERE  ...
> No more lineup :-(
> 
> 
> 
> Admittedly, when you type it yourself, it's a bit annoying, because you 
> can't use just tabs. But if it's generated, it won't do any harm.
> Why not giving PostgreSQL this extra portion of elegance...
> 
> Regards,
> Andreas
> 
> 

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Tue, 24 Jun 2003 15:00:47 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: pg_get_triggerdef in pg_dump
> SELECT foo
>   FROM bar b
>   LEFT JOIN chair c USING (thekeycol)
>  WHERE ...
> :-)


Sub-selects are much nicer:

   SELECT foo
        , bar
        , (SELECT anotherfoo
             FROM tab2
            WHERE tab2.col = tab1.col)
     FROM tab
     JOIN yet_another_table AS yat
          ON (yat.c = tab.c)
LEFT JOIN tab1 USING (anothercol)
    WHERE stuff IS TRUE
      AND  ( optional IS NULL
          OR optional > 5)
   HAVING count(*) > (SELECT total
                        FROM total_table)
 ORDER BY foo
 GROUP BY foo
        , bar
        , 3;

-- 
Rod Taylor <rbt@rbt.ca>

PGP Key: http://www.rbt.ca/rbtpub.asc
24 Jun 2003 19:07:53 +0000
Rod Taylor <rbt@rbt.ca>
Re: pg_get_triggerdef in pg_dump
Bruce Momjian wrote:

>I don't think an option to do such justification would be good unless we
>can do it consistenly in the code, and there is enough demand.
>
It's no big additional effort to do this, and going back and forth is 
not a big problem. I wouldn't invent an option for that, just do it. 
Let's see what's happening. At least, there seems agreement on not using 
tabs but spaces.

Still, I'd like to have somebody having a look at my algorithm. It's the 
key component, which should be proven right theoretically.

Regards,
Andreas



Tue, 24 Jun 2003 23:45:13 +0200
Andreas Pflug <Andreas.Pflug@web.de>
Re: pg_get_triggerdef in pg_dump
Bruce Momjian wrote:

>OK, added to TODO:
>
>	Modify pg_get_triggerdef() to take a boolean to pretty-print,
>	and use that as part of pg_dump along with psql
>
>Andreas, can you work on this?  I like the idea of removing extra
>parens, and merging it into the existing code rather than into contrib
>makes sense.
>  
>
Just an announcement: I'll be sending a patch for ruleutils.c and 
pg_proc.h tomorrow, after I performed some further testing.

pg_get_ruledef, pg_get_viewdef, pg_get_viewdef_name, pg_get_indexdef, 
pg_get_constraintdef and pg_get_expr get an additional parameter int4 
each which controls pretty-print (0: none, 1: parentheses, 1: 
indentation, 3: both).
I had to make several conditionals for the old parenthesing code, but I 
believe the functions still generate as usual if pretty-print is disabled.

At the moment, I assigned oids 2504-2509 (last used was 2503 when I 
updated from cvs) to the additional functions, is that ok?

Regards,
Andreas


Sun, 29 Jun 2003 02:12:36 +0200
Andreas Pflug <Andreas.Pflug@web.de>
Re: pg_get_triggerdef in pg_dump