Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unbreak $rs->create() with empty hashref on Oracle #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lasse-unity3d
Copy link

Oracle does not support the INSERT INTO ... DEFAULT VALUES syntax used by
SQLMaker by default. Furthermore, Oracle has no way of inserting a row
without specifying any columns, so the only reasonably fix seems to be to
pick a column and supply the DEFAULT keyword as value for it.

This approach seems to work without problems, even for things like
sequence+trigger-as-auto-increment.

unless (%$to_insert)
{
my ($col) = $source->columns;
$to_insert->{$col} = \'DEFAULT';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't modify the caller's hashref, just do $to_insert = { $col => \'DEFAULT' }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Will fix.

Oracle does not support the INSERT INTO ... DEFAULT VALUES syntax used by
SQLMaker by default. Furthermore, Oracle has no way of inserting a row
without specifying any columns, so the only reasonably fix seems to be to
pick a column and supply the DEFAULT keyword as value for it.

This approach seems to work without problems, even for things like
sequence+trigger-as-auto-increment.
@lasse-unity3d
Copy link
Author

2nd attempt - fixed accidental stepping on caller's hashref in insert() as pointed out by @ilmari .

@ilmari
Copy link
Contributor

ilmari commented Sep 30, 2014

Merged manually to blead as 236b59c

@ilmari ilmari closed this Sep 30, 2014
@ribasushi
Copy link
Collaborator

Hi @lasse-unity3d

I had a chance to review your contribution after it was merged to the dev branch, and found some issues. Please read the detailed explanation here: f8135512c

It would be great if you could spend some time to get the thing into shape, however given it has been a while it may be the case you no longer have the opportunity to work on this. Please let us know if this is the case, so someone else can pick it up and slog it to completion.

In any case: thanks for helping us make DBIC better ;)

@ribasushi ribasushi reopened this Oct 22, 2014
@lasse-unity3d
Copy link
Author

On Wed, Oct 22, 2014 at 9:57 PM, Peter Rabbitson notifications@github.com
wrote:

Hi @lasse-unity3d https://github.com/lasse-unity3d

I had a chance to review your contribution after it was merged to the dev
branch, and found some issues. Please read the detailed explanation here:
f813551 f8135512c

Thanks for the explanation. I'm not nearly familiar enough with DBIC
internals yet to really grasp the implications of what you explained but
I'll try to read up on it.

Do you have concrete examples of breakage resulting from the patch? That
would be really helpful for me to understand the problem better...

It would be great if you could spend some time to get the thing into
shape, however given it has been a while it may be the case you no longer
have the opportunity to work on this. Please let us know if this is the
case, so someone else can pick it up and slog it to completion.

The issue is still relevant to me but since our porting to Oracle has been
postponed it's not my focus at the moment. We're in a bit of a crunch
currently but I'll try to get around to this again soon...

Vacation next week may or may not help with this... :-)

/L

In any case: thanks for helping us make DBIC better ;)


Reply to this email directly or view it on GitHub
#61 (comment).

@ribasushi
Copy link
Collaborator

This patch uncovered extra irregularities in the base insert() codepth. I am still sifting through those, so the above has been put on hold.

I just wanted to update you that this PR is in my todo list, and will be merged in some manner before the next stable release (likely some time end of Jan).

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants