Noah,
* Noah Misch (n...@leadboat.com) wrote:
On Mon, Feb 14, 2011 at 04:06:59PM -0500, Stephen Frost wrote:
I've attached a new version of the patch that attempts to flesh out the
comments
based on your feedback. Does it improve things?
Yes, much better, thanks!
Offhand, I can't think
On Sun, Feb 13, 2011 at 02:53:07PM -0500, Robert Haas wrote:
On Sun, Feb 13, 2011 at 6:50 AM, Noah Misch n...@leadboat.com wrote:
Yikes.
?I didn't like that Assert much, but maybe we need it, because this is
scary.
Can you elaborate on the fear-inducing aspect? ?I don't mind re-adding
Noah,
I'm even less familiar w/ this code than Robert, but figured I'd give a
shot at reviewing this anyway. I definitely like avoiding table
rewrites if I can get away with it. :)
First question is- why do you use #ifdef USE_ASSERT_CHECKING ..?
assert_enabled exists and will work the way you
Hi Stephen,
Thanks for jumping in on this one.
On Mon, Feb 14, 2011 at 01:12:21PM -0500, Stephen Frost wrote:
First question is- why do you use #ifdef USE_ASSERT_CHECKING ..?
The other six code sites checking assert_enabled directly do the same.
assert_enabled exists and will work the way
* Noah Misch (n...@leadboat.com) wrote:
On Mon, Feb 14, 2011 at 01:12:21PM -0500, Stephen Frost wrote:
First question is- why do you use #ifdef USE_ASSERT_CHECKING ..?
The other six code sites checking assert_enabled directly do the same.
Wow, I could have sworn that I looked at what others
On Mon, Feb 14, 2011 at 04:06:59PM -0500, Stephen Frost wrote:
* Noah Misch (n...@leadboat.com) wrote:
On Mon, Feb 14, 2011 at 01:12:21PM -0500, Stephen Frost wrote:
In ATColumnChangeSetWorkLevel(), I'm really not a huge fan of using a
passed-in argument to move through a list with.. I'd
On Mon, Feb 14, 2011 at 7:52 AM, Noah Misch n...@leadboat.com wrote:
I'm half-tempted to put that part off to
9.2, in the hopes of getting a more substantial solution that can also
handle things like text - xml which we don't have time to re-engineer
right now.
I see.
After sleeping on it,
On Sun, Feb 13, 2011 at 12:04:20AM -0500, Robert Haas wrote:
On Sat, Feb 12, 2011 at 10:45 AM, Noah Misch n...@leadboat.com wrote:
That said, I've tried both constructions, and I marginally prefer the end
result
with AlteredTableInfo.verify. ?I've inlined ATColumnChangeRequiresRewrite
On Sun, Feb 13, 2011 at 6:50 AM, Noah Misch n...@leadboat.com wrote:
Yikes.
I didn't like that Assert much, but maybe we need it, because this is
scary.
Can you elaborate on the fear-inducing aspect? I don't mind re-adding the
Assert, but it seems that some positive understanding of the
On Fri, Feb 11, 2011 at 02:49:27PM -0500, Robert Haas wrote:
You might want to consider a second boolean in lieu of a three way
enum. I'm not sure if that's cleaner but if it lets you write:
if (blah)
at-verify = true;
instead of:
if (blah)
at-worklevel = Min(at-worklevel,
On Sat, Feb 12, 2011 at 10:45 AM, Noah Misch n...@leadboat.com wrote:
That said, I've tried both constructions, and I marginally prefer the end
result
with AlteredTableInfo.verify. I've inlined ATColumnChangeRequiresRewrite into
ATPrepAlterColumnType; it would need to either pass back two
On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch n...@leadboat.com wrote:
On Wed, Jan 26, 2011 at 07:31:40AM -0500, Robert Haas wrote:
I'd also suggest that this big if-block you changed to a case
statement could just as well stay as an if-block. There are only
three cases, and we want to avoid
Hi Robert,
On Fri, Feb 11, 2011 at 10:27:11AM -0500, Robert Haas wrote:
On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch n...@leadboat.com wrote:
Done as attached. ?This preserves compatibility with our current handling of
composite type dependencies. ?The rest you've seen before.
OK, I
On Fri, Feb 11, 2011 at 1:08 PM, Noah Misch n...@leadboat.com wrote:
It's a nice, clean patch. However, it achieves that by targeting a smaller
goal. Specifically, it omits:
1) Test cases and DEBUG messages sufficient to facilitate them
That was an intentional omission.
2) Skip rewrite
On Fri, Feb 11, 2011 at 1:08 PM, Noah Misch n...@leadboat.com wrote:
Even supposing we push off all scan-only cases to another patch, it would be
good to have the tablecmds.c-internal representation of that in mind. No
sense
in simplifying a 12-line change to an 8-line change, only to redo
On Fri, Feb 11, 2011 at 01:55:45PM -0500, Robert Haas wrote:
On Fri, Feb 11, 2011 at 1:08 PM, Noah Misch n...@leadboat.com wrote:
Even supposing we push off all scan-only cases to another patch, it would be
good to have the tablecmds.c-internal representation of that in mind. ?No
sense
On Fri, Feb 11, 2011 at 2:17 PM, Noah Misch n...@leadboat.com wrote:
Good to know. I can envision that perspective, and I share it when the
savings
is rather more substantial, say 10% of the patch or 100 lines. Below that
threshold, the energy I expend grasping two interface changes in one
On Sun, Feb 06, 2011 at 02:15:47AM -0500, Robert Haas wrote:
On Sat, Feb 5, 2011 at 7:44 PM, Noah Misch n...@leadboat.com wrote:
But this is a little unsatisfying, for two reasons. ?First, the error
message will be subtly wrong: we can make it complain about a table or
a type, but not a
On Sun, Feb 6, 2011 at 4:15 AM, Noah Misch n...@leadboat.com wrote:
That didn't quite work, because there's a caller in typecmds.c that
doesn't have the relation handy. So I made it take a relkind and a
name, which works fine.
Hmm, indeed. In get_rels_with_domain(), it's a scalar type.
On Sun, Feb 06, 2011 at 07:54:52AM -0500, Robert Haas wrote:
On Sun, Feb 6, 2011 at 4:15 AM, Noah Misch n...@leadboat.com wrote:
That didn't quite work, because there's a caller in typecmds.c that
doesn't have the relation handy. ?So I made it take a relkind and a
name, which works fine.
On Sun, Feb 06, 2011 at 08:40:44AM -0500, Noah Misch wrote:
On Sun, Feb 06, 2011 at 07:54:52AM -0500, Robert Haas wrote:
Yeeah, that's actually a little ugly. It's actually a domain
over a composite type, not a composite type proper, IIUC. Better
ideas?
There are no domains over
On Sun, Feb 6, 2011 at 8:58 AM, Noah Misch n...@leadboat.com wrote:
On Sun, Feb 06, 2011 at 08:40:44AM -0500, Noah Misch wrote:
On Sun, Feb 06, 2011 at 07:54:52AM -0500, Robert Haas wrote:
Yeeah, that's actually a little ugly. It's actually a domain
over a composite type, not a
On Sun, Feb 6, 2011 at 12:13 PM, Robert Haas robertmh...@gmail.com wrote:
That's not quite so good for translators, I think.
Another option is that we could just say relation (table, foreign
table, etc...) or type. We use the word relation as a more generic
version of table in a few other
On Sun, Feb 06, 2011 at 12:54:19PM -0500, Robert Haas wrote:
On Sun, Feb 6, 2011 at 12:13 PM, Robert Haas robertmh...@gmail.com wrote:
That's not quite so good for translators, I think.
Another option is that we could just say relation (table, foreign
table, etc...) or type. ?We use the
On Sun, Feb 6, 2011 at 8:18 PM, Noah Misch n...@leadboat.com wrote:
Or how about passing an ObjectType? Then we could specify
OBJECT_TABLE, OBJECT_FOREIGN_TABLE, or OBJECT_TYPE.
Could this be done without a several-line blob of code at each call site to
determine the answer? If and only if
On Mon, Feb 07, 2011 at 12:04:02AM -0500, Robert Haas wrote:
On Sun, Feb 6, 2011 at 8:18 PM, Noah Misch n...@leadboat.com wrote:
Or how about passing an ObjectType? ?Then we could specify
OBJECT_TABLE, OBJECT_FOREIGN_TABLE, or OBJECT_TYPE.
Could this be done without a several-line blob of
Robert,
Thanks for the obviously thought-out review.
On Sat, Feb 05, 2011 at 01:29:35AM -0500, Robert Haas wrote:
On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch n...@leadboat.com wrote:
Done as attached. ?This preserves compatibility with our current handling of
composite type dependencies.
On Sat, Feb 5, 2011 at 3:05 AM, Noah Misch n...@leadboat.com wrote:
Correct. It's perhaps obvious, but rewriting the table will always reindex
it.
Right.
3. If we're changing the data type of a column in the table, reindex the
table.
Rebuild indexes that depend on a changing column.
On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch n...@leadboat.com wrote:
Done as attached.
Looking at this still more, it appears that independent of any change
this patch may wish to make, there's a live bug here related to the
foreign table patch I committed back in December. Creating a foreign
On Sat, Feb 05, 2011 at 10:03:59AM -0500, Robert Haas wrote:
Looking at this still more, it appears that independent of any change
this patch may wish to make, there's a live bug here related to the
foreign table patch I committed back in December. Creating a foreign
table creates an
On Sat, Feb 5, 2011 at 7:44 PM, Noah Misch n...@leadboat.com wrote:
But this is a little unsatisfying, for two reasons. First, the error
message will be subtly wrong: we can make it complain about a table or
a type, but not a foreign table. At a quick look, it likes the right
fix might be to
On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch n...@leadboat.com wrote:
Done as attached. This preserves compatibility with our current handling of
composite type dependencies. The rest you've seen before.
OK, so I took a look at this in more detail today. The current logic
for table rewrites
On Wed, Jan 26, 2011 at 07:31:40AM -0500, Robert Haas wrote:
I'd also suggest that this big if-block you changed to a case
statement could just as well stay as an if-block. There are only
three cases, and we want to avoid rearranging things more than
necessary. It complicates both review and
On Tue, Jan 25, 2011 at 10:22 PM, Noah Misch n...@leadboat.com wrote:
I'm fine with this patch.
OK, committed.
The next patch removed new_changeoids, so we would instead be keeping it with
this as the only place referencing it.
[...]
The at2v2 patch would then morph to do something like:
On Mon, Jan 24, 2011 at 7:10 PM, Noah Misch n...@leadboat.com wrote:
* at1.1-default-composite.patch
Remove the error when the user adds a column with a default value to a table
whose rowtype is used in a column elsewhere.
Can we fix this without moving the logic around quite so much? I'm
On Mon, Jan 24, 2011 at 7:10 PM, Noah Misch n...@leadboat.com wrote:
* at1.2-doc-set-data-type.patch
The documentation used ALTER TYPE when it meant SET DATA TYPE, a subform
of
ALTER TABLE or ALTER FOREIGN TABLE. Fixes just that.
Committed this part. For reasons involving me being tired, I
On Tue, Jan 25, 2011 at 06:40:08PM -0500, Robert Haas wrote:
On Mon, Jan 24, 2011 at 7:10 PM, Noah Misch n...@leadboat.com wrote:
* at1.1-default-composite.patch
Remove the error when the user adds a column with a default value to a table
whose rowtype is used in a column elsewhere.
Can
Robert Haas robertmh...@gmail.com writes:
On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch n...@leadboat.com wrote:
As unintended fallout, it's no longer an error to add oids or a column with a
default value to a table whose rowtype is used in columns elsewhere. This
seems
best. Defaults on the
On Sun, Jan 23, 2011 at 12:06 PM, Tom Lane t...@sss.pgh.pa.us wrote:
Robert Haas robertmh...@gmail.com writes:
On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch n...@leadboat.com wrote:
As unintended fallout, it's no longer an error to add oids or a column with
a
default value to a table whose
On Sun, Jan 23, 2011 at 12:06:43PM -0500, Tom Lane wrote:
Robert Haas robertmh...@gmail.com writes:
On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch n...@leadboat.com wrote:
As unintended fallout, it's no longer an error to add oids or a column
with a
default value to a table whose rowtype is
On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch n...@leadboat.com wrote:
This patch removes ALTER TYPE rewrites in cases we can already prove valid. I
add a function GetCoerceExemptions() that walks an Expr according to rules
discussed in the design thread, simplified slightly pending additions in
41 matches
Mail list logo