Bad indentation for DECLARE in PostgreSQL

[sfllaw]

The DECLARE statement in PL/pgSQL is similar to the one in PL/SQL and is used to declare variables: https://www.postgresql.org/docs/current/plpgsql-declarations.html

Test case in PostgreSQL:

-- Current behaviour
CREATE FUNCTION less_than(a text, b text) RETURNS boolean AS $$
  DECLARE
  local_a text := a;
  local_b text := b;
  BEGIN
    RETURN local_a < local_b;
  END;
$$ LANGUAGE plpgsql;

-- Expected behaviour
CREATE FUNCTION less_than(a text, b text) RETURNS boolean AS $$
  DECLARE
    local_a text := a;
    local_b text := b;
  BEGIN
    RETURN local_a < local_b;
  END;
$$ LANGUAGE plpgsql;

However, there is also a DECLARE statement that creates a cursor: https://www.postgresql.org/docs/12/sql-declare.html

-- Current behaviour
DECLARE
liahona
  CURSOR FOR
	   SELECT *
	   FROM films;

-- Expected
DECLARE
  liahona
  CURSOR FOR
    SELECT *
      FROM films;

I think sqlind-beginning-of-block probably needs to handle the PostgreSQL case:

emacs-sql-indent/sql-indent.el

Lines 956 to 957 in 56be397

	(when (eq sql-product 'oracle) ; declare statements only start blocks in PL/SQL
	(sqlind-maybe-declare-statement))

However, for the cursor use-case, I think that sqlind-maybe-declare-statement needs to understand if it’s in-begin-block and do something special, especially in order to recognize and indent the embedded SELECT or VALUES statement.

[alex-hhh]

Thanks for reporting this. I pushed a fix, but unfortunately this had to change how existing indentations work, hopefully for the better. PRs #88, #89 and #67 are affected by this change.

Can you please check to see if it is OK for your code?

[sfllaw]

First, thank you for working on this so quickly.

However, it looks like there are two corner cases that aren’t covered.

The first is when a DECLARE follows another DECLARE, which I think can be detected by just checking for declare-statement:

-- Current behaviour
CREATE FUNCTION less_than(a text, b text) RETURNS boolean AS $$
  DECLARE local_a text := a;
    DECLARE local_b text := b;
  BEGIN
    RETURN local_a < local_b;
  END;
$$ LANGUAGE plpgsql;

-- Expected behaviour
CREATE FUNCTION less_than(a text, b text) RETURNS boolean AS $$
  DECLARE local_a text := a;
  DECLARE local_b text := b;
  BEGIN
    RETURN local_a < local_b;
  END;
$$ LANGUAGE plpgsql;

The second happens with nesting inside another BEGIN:

-- Current behaviour
CREATE FUNCTION less_than(a text, b text) RETURNS boolean AS $$
  BEGIN
    DECLARE
      local_a text := a;
    local_b text := b;
    BEGIN
      RETURN local_a < local_b;
    END;
  END;
$$ LANGUAGE plpgsql;

-- Expected behaviour
CREATE FUNCTION less_than(a text, b text) RETURNS boolean AS $$
  BEGIN
    DECLARE
      local_a text := a;
      local_b text := b;
    BEGIN
      RETURN local_a < local_b;
    END;
  END;
$$ LANGUAGE plpgsql;

This might require a new sqlind-in-function defun to handle all the cases where another DECLARE can happen (i.e. as the first block, within another block, inside a conditional, etc.):

-- Current behaviour
CREATE FUNCTION less_than(a text, b text) RETURNS boolean AS $$
  BEGIN
    IF a IS NULL THEN
      RETURN FALSE;
    ELSE
      DECLARE
	local_a text := a;
      local_b text := b;
      BEGIN
	RETURN local_a < local_b;
      END;
    END IF;
  END;
$$ LANGUAGE plpgsql;

-- Expected behaviour
CREATE FUNCTION less_than(a text, b text) RETURNS boolean AS $$
  BEGIN
    IF a IS NULL or b IS NULL THEN
      RETURN FALSE;
    ELSE
      DECLARE
	local_a text := a;
	local_b text := b;
      BEGIN
	RETURN local_a < local_b;
      END;
    END IF;
  END;
$$ LANGUAGE plpgsql;

[alex-hhh]

Thanks for preparing the test cases, I don't use Postgresql and I rely on these to improve sql-indent. I have pushed a fix for the test cases you prepared. Can you please have a look? Thanks, Alex.

[sfllaw]

I appreciate the effort to support Postgres!

Sadly, I have found a corner case that results from only examining the previous token: e7795c7#r37366259

[alex-hhh]

Sadly, I have found a corner case that results from only examining the previous token

Unlike an SQL parser, sql-indent cannot afford to parse the entire file from the start every time the user wants to indent a line, this would be too slow. Instead, sql-indent relies on searching backwards for a good starting point plus a collection of heuristics to determine the context of the line, so it can be indented. This mechanism means that you can easily find many corner cases which don't work correctly. I am trying to handle all reasonable cases, but I have limited time to work on this package and for the time being, I will just document this in the Limitations section of sql-indent.org.

sqlind-maybe-declare-statement is the sqlind-in-declare function you suggested, but I am not sure how it should be implemented: I need to come up with a mechanism to determine if the "declare" keyword starts a block with multiple declarations, or it is just a single statement.

I just want to be clear that I think your report is valid, but unfortunately I don't know how to fix it and have no more time to look into it.