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

fix: Fix crash caused by trigger function and bytea column fixes #63 #64

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

Conversation

davecramer
Copy link
Collaborator

This stops the crash from happening but I'm unclear that we should be stuffing a string into raw.

comments ?

@jconway
Copy link
Collaborator

jconway commented Jun 21, 2019

I'll try to look at this over the weekend (likely not until Sunday). The main question in my mind is why has this not been reported before -- i.e. is it a long standing latent bug that has never been reported before or is it something new induced by changes in postgres. And if the latter, what changes caused it.

@davecramer
Copy link
Collaborator Author

Looks like a long standing latent bug. I looked at git history and of that file and it seems we have been treating BYTEA and TEXT the same way there forever. Pretty sure this is not the solution but I'm unclear how to serialize a null into raw. Thanks for looking at it.

@mlt
Copy link
Collaborator

mlt commented Jul 3, 2019

It would be nice to test this as well.

I'm unclear how to serialize a null into raw

Isn't NULL just R_NilValue at all times?

I'm unclear that we should be stuffing a string into raw

I feel like it should be as verbatim as possible. So if string is hex or escape encoded representation for bytea, it should be decoded back and then, perhaps, be unserialized.

I started an attempt to do R to PG data passing without intermediate C string representation. I think the same ideally should be done when passing from PG to R as well (sans ALTREP).

@davecramer
Copy link
Collaborator Author

davecramer commented Jul 3, 2019 via email

@mlt
Copy link
Collaborator

mlt commented Jul 3, 2019

whatever is in the bytea has to be the result of serialize

I agree! I think the behavior should be consistent with pg_scalar_get_r

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

Successfully merging this pull request may close these issues.

3 participants