Re: identity columns

On 2017-03-29 13:40:29 -0400, Peter Eisentraut wrote:
> On 3/24/17 05:29, Vitaly Burovoy wrote:
> > It would be great if "DROP IDENTITY IF EXISTS" is in the current patch
> > since we don't have any disagreements about "DROP IDENTITY" behavior
> > and easiness of implementation of "IF EXISTS" there.
>
> Here is an updated patch that adds DROP IDENTITY IF EXISTS.
>
> Unfortunately, implementing ADD IF NOT EXISTS is much harder, so I
> haven't done that here.
>
> Additionally, this patch fixes a few minor issues that you had pointed
> out, and merges with the new expression evaluation system in the executor.

Your attachment has a mimetype of invalid/octet-stream - that's a first
for me ;)

Are you going to try to merge this soon, or are you pushing this to 11?
Feels a bit large for 10 at this point.

> + case T_NextValueExpr:
> + {
> + NextValueExpr *nve = (NextValueExpr *) node;
> +
> + scratch.opcode = EEOP_NEXTVALUEEXPR;
> + scratch.d.nextvalueexpr.seqid = nve->seqid;
> +
> + ExprEvalPushStep(state, &scratch);
> + break;
> + }

Hm - that's probably been answered somewhere, but why do we need a
special expression type for this?

> default:
> elog(ERROR, "unrecognized node type: %d",
> (int) nodeTag(node));
> diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
> index 4fbb5c1e74..5935b9ef75 100644
> --- a/src/backend/executor/execExprInterp.c
> +++ b/src/backend/executor/execExprInterp.c
> @@ -60,6 +60,7 @@
>
> #include "access/tuptoaster.h"
> #include "catalog/pg_type.h"
> +#include "commands/sequence.h"
> #include "executor/execExpr.h"
> #include "executor/nodeSubplan.h"
> #include "funcapi.h"
> @@ -337,6 +338,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
> &&CASE_EEOP_NULLIF,
> &&CASE_EEOP_SQLVALUEFUNCTION,
> &&CASE_EEOP_CURRENTOFEXPR,
> + &&CASE_EEOP_NEXTVALUEEXPR,
> &&CASE_EEOP_ARRAYEXPR,
> &&CASE_EEOP_ARRAYCOERCE,
> &&CASE_EEOP_ROW,
> @@ -1228,6 +1230,14 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
> EEO_NEXT();
> }
>
> + EEO_CASE(EEOP_NEXTVALUEEXPR)
> + {
> + *op->resvalue = Int64GetDatum(nextval_internal(op->d.nextvalueexpr.seqid, false));
> + *op->resnull = false;
> +
> + EEO_NEXT();
> + }

Is it guaranteed that the caller expects an int64? I saw that
nextvalueexpr's have a typeid field.

Greetings,

Andres Freund